[Asterisk-code-review] app voicemail: always copy dynamic struct to avoid race cond... (asterisk[13])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: app_voicemail: always copy dynamic struct to avoid race condition
......................................................................


Patch Set 10:

So, I tried to fix this up and ran into a number of problems.

First, the reason why this is still crashing in find_user is because *cur may or may not allocated on the heap. As a result, when we unilaterally ast_free the email fields that were copied onto vmu, we attempt to free memory that was allocated on the stack. Yikes. This fixes that:

-                       ast_free(vmu->email);
+                       if (ast_test_flag(cur, VM_ALLOCED)) {
+                           ast_free(vmu->email);
+                           ast_free(vmu->emailbody);
+                           ast_free(vmu->emailsubject);
+                       }
                        vmu->email = ast_strdup(cur->email);
-                       ast_free(vmu->emailbody);
                        vmu->emailbody = ast_strdup(cur->emailbody);
-                       ast_free(vmu->emailsubject);

Unfortunately, that leads to another new problem: apparently free_user can now be called with a NULL vmu pointer. This probably didn't happen in the past because we allowed a vmu to be allocated on the stack in some fashion, although this honestly just scares me (were we passing pointers to stack frames that were already out of scope? Who knows!). A simple check in free_user suffices:

 static void free_user(struct ast_vm_user *vmu)
 {
+       if (!vmu) {
+           return;
+       }
+
        ast_free(vmu->email);

With this in place, we now crash in a few spots where vmu->email was previously non-NULL (but probably pointed to garbage) and where - thanks to the ast_calloc - it is now NULL. This means we need to make the following changes:

@@ -12413,7 +12419,7 @@ static int acf_vm_info(struct ast_channel *chan, const char *cmd, char *args, ch
                } else if (!strncasecmp(arg.attribute, "fullname", 8)) {
                        ast_copy_string(buf, vmu->fullname, len);
                } else if (!strncasecmp(arg.attribute, "email", 5)) {
-                       ast_copy_string(buf, vmu->email, len);
+                       ast_copy_string(buf, S_OR(vmu->email, ""), len);
                } else if (!strncasecmp(arg.attribute, "pager", 5)) {
                        ast_copy_string(buf, vmu->pager, len);
                } else if (!strncasecmp(arg.attribute, "language", 8)) {

And:

@@ -13160,7 +13166,7 @@ static int manager_list_voicemail_users(struct mansession *s, const struct messa
                        vmu->context,
                        vmu->mailbox,
                        vmu->fullname,
-                       vmu->email,
+                       S_OR(vmu->email, ""),
                        vmu->pager,
                        ast_strlen_zero(vmu->serveremail) ? serveremail : vmu->serveremail,
                        mailcmd,


With these changes in place, the unit tests no longer crash. However, they also now fail.

In particular:

*CLI> test execute category /apps/app_voicemail/
Running all available tests matching category /apps/app_voicemail/

START  /apps/app_voicemail/ - test_voicemail_vm_info 
[app_voicemail.c:test_voicemail_vm_info:14637]: VM_INFO respose was: '', but expected: 'vm-info-test at example.net'
[app_voicemail.c:test_voicemail_vm_info:14637]: VM_INFO respose was: '', but expected: 'Test Framework Mailbox'
[app_voicemail.c:test_voicemail_vm_info:14637]: VM_INFO respose was: '', but expected: 'vm-info-pager-test at example.net'
[app_voicemail.c:test_voicemail_vm_info:14637]: VM_INFO respose was: '', but expected: 'en_US'
[app_voicemail.c:test_voicemail_vm_info:14637]: VM_INFO respose was: '', but expected: 'central'
[app_voicemail.c:test_voicemail_vm_info:14637]: VM_INFO respose was: '', but expected: 'en'
[app_voicemail.c:test_voicemail_vm_info:14637]: VM_INFO respose was: '', but expected: '9876'
END    /apps/app_voicemail/ - test_voicemail_vm_info Time: <1ms Result: FAIL
START  /apps/app_voicemail/ - test_voicemail_load_config 
  == Parsing '/tmp/voicemail.conf.4c72JQ': Found
END    /apps/app_voicemail/ - test_voicemail_load_config Time: <1ms Result: PASS
START  /apps/app_voicemail/ - test_voicemail_notify_endl 
END    /apps/app_voicemail/ - test_voicemail_notify_endl Time: 7ms Result: PASS
START  /apps/app_voicemail/ - vmuser 
END    /apps/app_voicemail/ - vmuser Time: <1ms Result: PASS
START  /apps/app_voicemail/ - test_voicemail_msgcount 
END    /apps/app_voicemail/ - test_voicemail_msgcount Time: 5ms Result: PASS
START  /apps/app_voicemail/ - vmsayname_exec 
[app_voicemail.c:test_voicemail_vmsayname:14166]: Test playing of extension when greeting is not available...
END    /apps/app_voicemail/ - vmsayname_exec Time: <1ms Result: FAIL

6 Test(s) Executed  4 Passed  2 Failed


Disregarding vmsayname_exec, test_voicemail_vm_info is failing with all fields retrieved in the VM_INFO function now being empty. I suspect this is because we have changed the stack/heap semantics that this function expected vmu structs to have. That means that with this patch, even though we may fix the crashes it introduced and some obvious glitches, there are some unintended ripple effects that probably need to be thought through more.

-- 
To view, visit https://gerrit.asterisk.org/2433
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a0643813116da84e2617291903d0d489b7425fb
Gerrit-PatchSet: 10
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: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: No