app_voicemail: Fix unchecked bounds of myArray in IMAP_STORAGE.

In update_messages_by_imapuser(), messages were appended to a finite
array which resulted in a crash when an IMAP mailbox contained more
than 256 entries. This memory is now dynamically increased as needed.

Observe that this patch adds a bunch of XXX's to questionable code. See
the review (url below) for more information.

ASTERISK-24190 #close
Reported by: Nick Adams
Tested by: Nick Adams

Review: https://reviewboard.asterisk.org/r/4126/
........

Merged revisions 426691 from http://svn.asterisk.org/svn/asterisk/branches/1.8


git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/11@426692 65c4cc65-6c06-0410-ace0-fbb531ad65f3
changes/61/61/1
Walter Doekes 11 years ago
parent 865aa54aac
commit 15f16e3187

@ -814,7 +814,8 @@ struct vm_state {
#ifdef IMAP_STORAGE #ifdef IMAP_STORAGE
ast_mutex_t lock; ast_mutex_t lock;
int updated; /*!< decremented on each mail check until 1 -allows delay */ int updated; /*!< decremented on each mail check until 1 -allows delay */
long msgArray[VMSTATE_MAX_MSG_ARRAY]; long *msgArray;
unsigned msg_array_max;
MAILSTREAM *mailstream; MAILSTREAM *mailstream;
int vmArrayIndex; int vmArrayIndex;
char imapuser[80]; /*!< IMAP server login */ char imapuser[80]; /*!< IMAP server login */
@ -2352,6 +2353,7 @@ static int __messagecount(const char *context, const char *mailbox, const char *
free_user(vmu); free_user(vmu);
return -1; return -1;
} }
ast_assert(msgnum < vms->msg_array_max);
/* check if someone is accessing this box right now... */ /* check if someone is accessing this box right now... */
vms_p = get_vm_state_by_imapuser(vmu->imapuser, 1); vms_p = get_vm_state_by_imapuser(vmu->imapuser, 1);
@ -2981,6 +2983,17 @@ static void update_messages_by_imapuser(const char *user, unsigned long number)
} }
ast_debug(3, "saving mailbox message number %lu as message %d. Interactive set to %d\n", number, vms->vmArrayIndex, vms->interactive); ast_debug(3, "saving mailbox message number %lu as message %d. Interactive set to %d\n", number, vms->vmArrayIndex, vms->interactive);
/* Ensure we have room for the next message. */
if (vms->vmArrayIndex >= vms->msg_array_max) {
long *new_mem = ast_realloc(vms->msgArray, 2 * vms->msg_array_max * sizeof(long));
if (!new_mem) {
return;
}
vms->msgArray = new_mem;
vms->msg_array_max *= 2;
}
vms->msgArray[vms->vmArrayIndex++] = number; vms->msgArray[vms->vmArrayIndex++] = number;
} }
@ -3258,6 +3271,7 @@ static struct vm_state *create_vm_state_from_user(struct ast_vm_user *vmu)
return vms_p; return vms_p;
} }
ast_debug(5, "Adding new vmstate for %s\n", vmu->imapuser); ast_debug(5, "Adding new vmstate for %s\n", vmu->imapuser);
/* XXX: Is this correctly freed always? */
if (!(vms_p = ast_calloc(1, sizeof(*vms_p)))) if (!(vms_p = ast_calloc(1, sizeof(*vms_p))))
return NULL; return NULL;
ast_copy_string(vms_p->imapuser, vmu->imapuser, sizeof(vms_p->imapuser)); ast_copy_string(vms_p->imapuser, vmu->imapuser, sizeof(vms_p->imapuser));
@ -3372,6 +3386,7 @@ static void vmstate_insert(struct vm_state *vms)
vms->newmessages = altvms->newmessages; vms->newmessages = altvms->newmessages;
vms->oldmessages = altvms->oldmessages; vms->oldmessages = altvms->oldmessages;
vms->vmArrayIndex = altvms->vmArrayIndex; vms->vmArrayIndex = altvms->vmArrayIndex;
/* XXX: no msgArray copying? */
vms->lastmsg = altvms->lastmsg; vms->lastmsg = altvms->lastmsg;
vms->curmsg = altvms->curmsg; vms->curmsg = altvms->curmsg;
/* get a pointer to the persistent store */ /* get a pointer to the persistent store */
@ -3430,10 +3445,14 @@ static void vmstate_delete(struct vm_state *vms)
if (vc) { if (vc) {
ast_mutex_destroy(&vc->vms->lock); ast_mutex_destroy(&vc->vms->lock);
ast_free(vc->vms->msgArray);
vc->vms->msgArray = NULL;
vc->vms->msg_array_max = 0;
/* XXX: is no one supposed to free vms itself? */
ast_free(vc); ast_free(vc);
} } else {
else
ast_log(AST_LOG_ERROR, "No vmstate found for user:%s, mailbox %s\n", vms->imapuser, vms->username); ast_log(AST_LOG_ERROR, "No vmstate found for user:%s, mailbox %s\n", vms->imapuser, vms->username);
}
} }
static void set_update(MAILSTREAM * stream) static void set_update(MAILSTREAM * stream)
@ -3455,11 +3474,13 @@ static void set_update(MAILSTREAM * stream)
static void init_vm_state(struct vm_state *vms) static void init_vm_state(struct vm_state *vms)
{ {
int x; vms->msg_array_max = VMSTATE_MAX_MSG_ARRAY;
vms->vmArrayIndex = 0; vms->msgArray = ast_calloc(vms->msg_array_max, sizeof(long));
for (x = 0; x < VMSTATE_MAX_MSG_ARRAY; x++) { if (!vms->msgArray) {
vms->msgArray[x] = 0; /* Out of mem? This can't be good. */
vms->msg_array_max = 0;
} }
vms->vmArrayIndex = 0;
ast_mutex_init(&vms->lock); ast_mutex_init(&vms->lock);
} }

Loading…
Cancel
Save