[Home]

Summary:ASTERISK-24786: [patch] - Asterisk terminates when playing a voicemail stored in LDAP
Reporter:Graham Barnett (GrahamJB)Labels:
Date Opened:2015-02-13 06:30:48.000-0600Date Closed:2015-03-10 13:12:12
Priority:CriticalRegression?
Status:Closed/CompleteComponents:Applications/app_voicemail/IMAP
Versions:13.1.0 13.2.0 Frequency of
Occurrence		Constant
Related
Issues:
is related toASTERISK-23990 [patch] app_voicemail crash when using IMAP greetings and forwarding messages
Environment:Attachments:( 0) app_voicemail.c.patch_terminate
( 1) app_voicemail.c.patchSIGSEGV3rev2
Description:Symptom
# If user has no greetings and user opens mailbox (*97) and tries to play a message, asterisk terminates
# If user has at least one greeting, playing back a message gives:
{noformat}
[14626][C-0000002c]: app_voicemail.c:8677 play_message: No origtime?!
{noformat}

Why?
{code}
vms_x = get_vm_state_by_imapuser(user, x)
{code}
caches existing states by user.
The states are looked up my imap version, user name, interactive=2 || interactive matches.

Why does this matter?
{code}
int fold = 0;
vms_p = get_vm_state_by_imapuser(vmu->imapuser, 0);
if (!vms_p) {
vms_p = get_vm_state_by_mailbox(mailbox, context, 1);
}
ret = init_mailstream(vms_p, fold);
{code}

vs
{code}
if (!(vmu = find_user(NULL, context, mailbox))) {
   RETRIEVE(tempfile, -1, vmu->mailbox, vmu->context);
   res = imap_retrieve_greeting(dir, msgnum, vmu);
   if (!(vms_p = get_vm_state_by_mailbox(vmu->mailbox, vmu->context, 1)) &&
   if (init_mailstream(vms_p, GREETINGS_FOLDER) || !vms_p->mailstream) {
{code}

i.e. the mailstream can be left pointed at the GREETINGS and not the Inbox

And why does that matter?
{code}
imap_retrieve_file
if (!(vms = get_vm_state_by_mailbox(vmu->mailbox, vmu->context, 1)) && !(vms = get_vm_state_by_mailbox(vmu->mailbox, vmu->context, 0))) {
{code}

*CAN BE WRONG MAILSTREAM !!!!!*
{code}
header_content = mail_fetchheader (vms->mailstream, vms->msgArray[msgnum]);
{code}

What is the fix?
We need to init the mailstream in imap_retrieve_file after getting the vm_state_by_mailbox

\[Edit: inline patch removed\]

Will post patch file later.
Comments:By: Matt Jordan (mjordan) 2015-02-13 09:25:03.914-0600

We cannot accept patches in issue descriptions or comments. Please attach the patch to the issue after signing a license contributor agreement. Thanks!
By: Graham Barnett (GrahamJB) 2015-02-13 11:53:15.718-0600

Patch attached
By: Matt Jordan (mjordan) 2015-02-20 10:45:19.566-0600

I'm not sure the patch here is the right solution.

The problem is that every time we retrieve a file, we are now going to switch it back to the INBOX folder, regardless of whether or not the file being played was a greetings. That would probably make it so that attempting to play a message from some folder other than the IMAP INBOX will always fail, which feels bad.

Why not switch the mailstream *back* to the INBOX in {{imap_retrieve_greeting}} - that is, move your patch to the end of {{imap_retrieve_greeting}}? That should have the same effect - the mailstream is no longer pointed at the GREETINGS folder - and you won't impact the normal retrieval of messages from folders other than the INBOX.
By: Graham Barnett (GrahamJB) 2015-02-20 14:25:23.990-0600

That would potentially solve the problem ... but is very hard to know if it will work in all cases, as the retrieve expects to be iterating the passed 'dir'
as in {{static int imap_retrieve_file (const char *dir, const int msgnum, const char *mailbox, const char *context);}}

What you really want to do is init the mailbox to the IMAP folder that corresponds with dir. Otherwise you are relying on other code in a non-obvious manner.

You could split dir into rootPath and relative path (and update vm_state in this way for all 4 folders moving to one root path and 4 relative paths).

What do you think would be safer and better coding?

By: Matt Jordan (mjordan) 2015-02-25 14:34:01.369-0600

I'm not sure it's quite as bad as you might think. The {{RETRIEVE}} macro knows that what is being obtained is a greeting based on the {{msgnum}} passed to it - that is, we know that if we get a {{-1}} in {{imap_retrieve_file}}, that we are going out to get a greeting and that the mailstream will be affected as a result.

After getting a greeting, we need to restore the mailstream back to its prior state. That should be possible, as the previous folder that the state was on should be stored in {{vms->curbox}}. Converting {{vms->curbox}} back to its integer representation for a new call to {{init_mailstream}} can be done using {{get_folder_by_name}}.

That is, after getting a greeting, we should probably have something like:

{code}

int folder_id = get_folder_by_name(vms->curbox);
if (folder_id < 0) {
   folder_id = 0;
}
init_mailstream(vms, folder_id);

{code}

The only other change I would make to your patch is to only call {{init_mailstream}} again if we did play a greeting - otherwise, I think the location you are doing it in is appropriate.
By: Graham Barnett (GrahamJB) 2015-03-01 09:26:25.391-0600

Attached revised fix app_voicemail.c.patchSIGSEGV3
By: Graham Barnett (GrahamJB) 2015-03-01 16:01:12.766-0600

Belt and braces version .... seems to be solid in my testing
By: Rusty Newton (rnewton) 2015-03-03 18:59:41.361-0600

Thanks Graham, you may consider going ahead and putting the patch up for code review: https://wiki.asterisk.org/wiki/display/AST/Code+Review
By: Matt Jordan (mjordan) 2015-03-10 13:13:20.076-0500

Merged into 11+. Thanks for the contribution!