[Asterisk-code-review] app voicemail: always copy dynamic struct to avoid race cond... (asterisk[13])
Joshua Colp
asteriskteam at digium.com
Tue May 3 19:14:32 CDT 2016
Joshua Colp has submitted this change and it was merged.
Change subject: app_voicemail: always copy dynamic struct to avoid race condition
......................................................................
app_voicemail: always copy dynamic struct to avoid race condition
Voicemail email addresses can be corrupt or voicemail
emails can end up being sent to the wrong email address if asterisk is
reading voicemail.conf during a reload and processing an email at the
same time. This patch always copies the struct that would otherwise only
be copied once.
ASTERISK-24463 #close
Reported by: John Campbell
Tested by: Etienne Lessard
Tested by: Andrew Nagy
Change-Id: I3a0643813116da84e2617291903d0d489b7425fb
---
M apps/app_voicemail.c
1 file changed, 79 insertions(+), 24 deletions(-)
Approvals:
Kevin Harwell: Looks good to me, but someone else must approve
Matt Jordan: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved; Verified
diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c
index a0b668d..c94513d 100644
--- a/apps/app_voicemail.c
+++ b/apps/app_voicemail.c
@@ -1725,13 +1725,14 @@
}
if (cur) {
/* Make a copy, so that on a reload, we have no race */
- if ((vmu = (ivm ? ivm : ast_malloc(sizeof(*vmu))))) {
+ if ((vmu = (ivm ? ivm : ast_calloc(1, sizeof(*vmu))))) {
+ ast_free(vmu->email);
+ ast_free(vmu->emailbody);
+ ast_free(vmu->emailsubject);
*vmu = *cur;
- if (!ivm) {
- vmu->email = ast_strdup(cur->email);
- vmu->emailbody = ast_strdup(cur->emailbody);
- vmu->emailsubject = ast_strdup(cur->emailsubject);
- }
+ vmu->email = ast_strdup(cur->email);
+ vmu->emailbody = ast_strdup(cur->emailbody);
+ vmu->emailsubject = ast_strdup(cur->emailsubject);
ast_set2_flag(vmu, !ivm, VM_ALLOCED);
AST_LIST_NEXT(vmu, list) = NULL;
}
@@ -2009,17 +2010,18 @@
static void free_user(struct ast_vm_user *vmu)
{
+ if (!vmu) {
+ return;
+ }
+
+ ast_free(vmu->email);
+ vmu->email = NULL;
+ ast_free(vmu->emailbody);
+ vmu->emailbody = NULL;
+ ast_free(vmu->emailsubject);
+ vmu->emailsubject = NULL;
+
if (ast_test_flag(vmu, VM_ALLOCED)) {
-
- ast_free(vmu->email);
- vmu->email = NULL;
-
- ast_free(vmu->emailbody);
- vmu->emailbody = NULL;
-
- ast_free(vmu->emailsubject);
- vmu->emailsubject = NULL;
-
ast_free(vmu);
}
}
@@ -2457,14 +2459,17 @@
return 0;
/* We have to get the user before we can open the stream! */
+ memset(&vmus, 0, sizeof(vmus));
vmu = find_user(&vmus, context, mailbox);
if (!vmu) {
ast_log(AST_LOG_WARNING, "Couldn't find mailbox %s in context %s\n", mailbox, context);
+ free_user(vmu);
return -1;
} else {
/* No IMAP account available */
if (vmu->imapuser[0] == '\0') {
ast_log(AST_LOG_WARNING, "IMAP user not set for mailbox %s\n", vmu->mailbox);
+ free_user(vmu);
return -1;
}
}
@@ -2484,9 +2489,11 @@
if (vms_p) {
ast_debug(3, "Returning before search - user is logged in\n");
if (fold == 0) { /* INBOX */
+ free_user(vmu);
return urgent ? vms_p->urgentmessages : vms_p->newmessages;
}
if (fold == 1) { /* Old messages */
+ free_user(vmu);
return vms_p->oldmessages;
}
}
@@ -2503,6 +2510,7 @@
ret = init_mailstream(vms_p, fold);
if (!vms_p->mailstream) {
ast_log(AST_LOG_ERROR, "Houston we have a problem - IMAP mailstream is NULL\n");
+ free_user(vmu);
return -1;
}
if (ret == 0) {
@@ -2546,6 +2554,7 @@
/*Freeing the searchpgm also frees the searchhdr*/
mail_free_searchpgm(&pgm);
ast_mutex_unlock(&vms_p->lock);
+ free_user(vmu);
vms_p->updated = 0;
return vms_p->vmArrayIndex;
} else {
@@ -2553,6 +2562,7 @@
mail_ping(vms_p->mailstream);
ast_mutex_unlock(&vms_p->lock);
}
+ free_user(vmu);
return 0;
}
@@ -6185,6 +6195,7 @@
return -1;
}
+ memset(&svm, 0, sizeof(svm));
if (!(recipient = find_user(&svm, recdata->context, recdata->mailbox))) {
ast_log(LOG_ERROR, "No entry in voicemail config file for '%s@%s'\n", recdata->mailbox, recdata->context);
return -1;
@@ -6500,6 +6511,7 @@
}
ast_debug(3, "Before find_user\n");
+ memset(&svm, 0, sizeof(svm));
if (!(vmu = find_user(&svm, context, ext))) {
ast_log(AST_LOG_WARNING, "No entry in voicemail config file for '%s'\n", ext);
pbx_builtin_setvar_helper(chan, "VMSTATUS", "FAILED");
@@ -6529,6 +6541,7 @@
snprintf(tempfile, sizeof(tempfile), "%s%s/%s/temp", VM_SPOOL_DIR, vmu->context, ext);
if ((res = create_dirpath(tmpdir, sizeof(tmpdir), vmu->context, ext, "tmp"))) {
ast_log(AST_LOG_WARNING, "Failed to make directory (%s)\n", tempfile);
+ free_user(vmu);
ast_free(tmp);
return -1;
}
@@ -6672,9 +6685,9 @@
}
ast_play_and_wait(chan, "transfer");
ast_channel_priority_set(chan, 0);
- free_user(vmu);
pbx_builtin_setvar_helper(chan, "VMSTATUS", "USEREXIT");
}
+ free_user(vmu);
ast_free(tmp);
return OPERATOR_EXIT;
}
@@ -6708,6 +6721,7 @@
res = inboxcount(ext_context, &newmsgs, &oldmsgs);
if (res < 0) {
ast_log(AST_LOG_NOTICE, "Can not leave voicemail, unable to count messages\n");
+ free_user(vmu);
ast_free(tmp);
return -1;
}
@@ -6718,6 +6732,7 @@
*/
if (!(vms = create_vm_state_from_user(vmu))) {
ast_log(AST_LOG_ERROR, "Couldn't allocate necessary space\n");
+ free_user(vmu);
ast_free(tmp);
return -1;
}
@@ -6912,6 +6927,7 @@
*cntx = '\0';
cntx++;
}
+ memset(&recipu, 0, sizeof(recipu));
if ((recip = find_user(&recipu, cntx, exten))) {
copy_message(chan, vmu, 0, msgnum, duration, recip, fmt, dir, flag, NULL);
free_user(recip);
@@ -10939,6 +10955,7 @@
}
ast_debug(1, "Before find user for mailbox %s\n", mailbox);
+ memset(&vmus, 0, sizeof(vmus));
vmu = find_user(&vmus, context, mailbox);
if (vmu && (vmu->password[0] == '\0' || (vmu->password[0] == '-' && vmu->password[1] == '\0'))) {
/* saved password is blank, so don't bother asking */
@@ -10946,10 +10963,12 @@
} else {
if (ast_streamfile(chan, vm_password, ast_channel_language(chan))) {
ast_log(AST_LOG_WARNING, "Unable to stream password file\n");
+ free_user(vmu);
return -1;
}
if (ast_readstring(chan, password, sizeof(password) - 1, 2000, 10000, "#") < 0) {
ast_log(AST_LOG_WARNING, "Unable to read password\n");
+ free_user(vmu);
return -1;
} else if (password[0] == '*') {
/* user entered '*' */
@@ -10957,11 +10976,13 @@
if (ast_exists_extension(chan, ast_channel_context(chan), "a", 1,
S_COR(ast_channel_caller(chan)->id.number.valid, ast_channel_caller(chan)->id.number.str, NULL))) {
mailbox[0] = '*';
+ free_user(vmu);
return -1;
}
ast_verb(4, "Jump to extension 'a' failed; setting mailbox and user to NULL\n");
mailbox[0] = '\0';
/* if the password entered was '*', do not let a user mailbox be created if the extension 'a' is not defined */
+ free_user(vmu);
vmu = NULL;
}
}
@@ -10982,6 +11003,7 @@
if (skipuser || logretries >= max_logins) {
if (ast_streamfile(chan, "vm-incorrect", ast_channel_language(chan))) {
ast_log(AST_LOG_WARNING, "Unable to stream incorrect message\n");
+ free_user(vmu);
return -1;
}
} else {
@@ -10989,16 +11011,20 @@
adsi_login(chan);
if (ast_streamfile(chan, "vm-incorrect-mailbox", ast_channel_language(chan))) {
ast_log(AST_LOG_WARNING, "Unable to stream incorrect mailbox message\n");
+ free_user(vmu);
return -1;
}
}
- if (ast_waitstream(chan, "")) /* Channel is hung up */
+ if (ast_waitstream(chan, "")) { /* Channel is hung up */
+ free_user(vmu);
return -1;
+ }
}
}
if (!valid && (logretries >= max_logins)) {
ast_stopstream(chan);
ast_play_and_wait(chan, "vm-goodbye");
+ free_user(vmu);
return -1;
}
if (vmu && !skipuser) {
@@ -11105,6 +11131,8 @@
vmstate_delete(&vms);
}
#endif
+
+ free_user(vmu);
return res;
}
@@ -12301,7 +12329,7 @@
static int vm_box_exists(struct ast_channel *chan, const char *data)
{
- struct ast_vm_user svm;
+ struct ast_vm_user svm, *vmu;
char *context, *box;
AST_DECLARE_APP_ARGS(args,
AST_APP_ARG(mbox);
@@ -12331,8 +12359,10 @@
context++;
}
- if (find_user(&svm, context, args.mbox)) {
+ vmu = find_user(&svm, context, args.mbox);
+ if (vmu) {
pbx_builtin_setvar_helper(chan, "VMBOXEXISTSSTATUS", "SUCCESS");
+ free_user(vmu);
} else
pbx_builtin_setvar_helper(chan, "VMBOXEXISTSSTATUS", "FAILED");
@@ -12341,7 +12371,7 @@
static int acf_mailbox_exists(struct ast_channel *chan, const char *cmd, char *args, char *buf, size_t len)
{
- struct ast_vm_user svm;
+ struct ast_vm_user svm, *vmu;
AST_DECLARE_APP_ARGS(arg,
AST_APP_ARG(mbox);
AST_APP_ARG(context);
@@ -12360,7 +12390,10 @@
ast_log(AST_LOG_WARNING, "MAILBOX_EXISTS is deprecated. Please use ${VM_INFO(%s,exists)} instead.\n", args);
}
- ast_copy_string(buf, find_user(&svm, ast_strlen_zero(arg.context) ? "default" : arg.context, arg.mbox) ? "1" : "0", len);
+ vmu = find_user(&svm, ast_strlen_zero(arg.context) ? "default" : arg.context, arg.mbox);
+ ast_copy_string(buf, vmu ? "1" : "0", len);
+ free_user(vmu);
+
return 0;
}
@@ -12396,10 +12429,12 @@
return -1;
}
+ memset(&svm, 0, sizeof(svm));
vmu = find_user(&svm, context, mailbox);
if (!strncasecmp(arg.attribute, "exists", 5)) {
ast_copy_string(buf, vmu ? "1" : "0", len);
+ free_user(vmu);
return 0;
}
@@ -12428,13 +12463,16 @@
res = messagecount(mailbox_id, arg.folder);
if (res < 0) {
ast_log(LOG_ERROR, "Unable to retrieve message count for mailbox %s\n", arg.mailbox_context);
+ free_user(vmu);
return -1;
}
snprintf(buf, len, "%d", res);
} else {
ast_log(LOG_ERROR, "Unknown attribute '%s' for VM_INFO\n", arg.attribute);
+ free_user(vmu);
return -1;
}
+ free_user(vmu);
}
return 0;
@@ -14248,6 +14286,7 @@
}
#endif
+ memset(&svm, 0, sizeof(svm));
if (!(vmu = find_user(&svm, testcontext, testmailbox)) &&
!(vmu = find_or_create(testcontext, testmailbox))) {
ast_test_status_update(test, "Cannot create vmu structure\n");
@@ -14277,6 +14316,7 @@
#ifdef IMAP_STORAGE
chan = ast_channel_unref(chan);
#endif
+ free_user(vmu);
return AST_TEST_FAIL;
}
}
@@ -14360,6 +14400,7 @@
syserr > 0 ? strerror(syserr) : "unable to fork()");
}
+ free_user(vmu);
return res;
}
@@ -14469,6 +14510,7 @@
}
}
fclose(file);
+ free_user(vmu);
return res;
}
@@ -14629,6 +14671,7 @@
}
chan = ast_channel_unref(chan);
+ free_user(vmu);
return res;
}
#endif /* defined(TEST_FRAMEWORK) */
@@ -15020,8 +15063,10 @@
ast_config_destroy(msg_cfg);
return res;
} else {
- struct ast_vm_user vmu2;
- if (find_user(&vmu2, vmu->context, num)) {
+ struct ast_vm_user vmu2, *vmu3;
+ memset(&vmu2, 0, sizeof(vmu2));
+ vmu3 = find_user(&vmu2, vmu->context, num);
+ if (vmu3) {
struct leave_vm_options leave_options;
char mailbox[AST_MAX_EXTENSION * 2 + 2];
snprintf(mailbox, sizeof(mailbox), "%s@%s", num, vmu->context);
@@ -15034,6 +15079,7 @@
if (!res)
res = 't';
ast_config_destroy(msg_cfg);
+ free_user(vmu3);
return res;
} else {
/* Sender has no mailbox, can't reply */
@@ -15528,11 +15574,13 @@
if (!(mailbox_snapshot = ast_calloc(1, sizeof(*mailbox_snapshot)))) {
ast_log(AST_LOG_ERROR, "Failed to allocate memory for mailbox snapshot\n");
+ free_user(vmu);
return NULL;
}
if (!(mailbox_snapshot->snapshots = ast_calloc(ARRAY_LEN(mailbox_folders), sizeof(*mailbox_snapshot->snapshots)))) {
ast_free(mailbox_snapshot);
+ free_user(vmu);
return NULL;
}
@@ -15593,6 +15641,7 @@
}
#endif
+ free_user(vmu);
return mailbox_snapshot;
}
@@ -15747,6 +15796,7 @@
if (!(to_vmu = find_user(&to_vmus, to_context, to_mailbox))) {
ast_log(LOG_WARNING, "Can't find voicemail user to forward to (%s@%s)\n", to_mailbox, to_context);
+ free_user(vmu);
return -1;
}
@@ -15827,6 +15877,8 @@
notify_new_state(to_vmu);
}
+ free_user(vmu);
+ free_user(to_vmu);
return res;
}
@@ -15930,6 +15982,7 @@
notify_new_state(vmu);
}
+ free_user(vmu);
return res;
}
@@ -16027,6 +16080,7 @@
notify_new_state(vmu);
}
+ free_user(vmu);
return res;
}
@@ -16140,6 +16194,7 @@
notify_new_state(vmu);
}
+ free_user(vmu);
return res;
}
--
To view, visit https://gerrit.asterisk.org/2433
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3a0643813116da84e2617291903d0d489b7425fb
Gerrit-PatchSet: 13
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Andrew Nagy <andrew.nagy at the159.com>
Gerrit-Reviewer: Andrew Nagy <andrew.nagy at the159.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: ibercom <ibercom123 at gmail.com>
More information about the asterisk-code-review
mailing list