[Home]

Summary:ASTERISK-12073: MWI event mailbox and context strings in mwist struct destroyed at ast_taskprocessor_push
Reporter:Tomo Takebe (tomo1657)Labels:
Date Opened:2008-05-23 12:23:46Date Closed:2008-05-23 16:15:40
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_voicemail
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 12712.patch
Description:When mwi_sub_event_cb() calls ast_taskprocessor_push(), the strings "mwist->mailbox" and "mwist->context" are no longer pointing to reliable memory when handle_subscribe() is called in the processor when *datap is accessed to retrieve the strings.

This results in messagecount() looking for mailboxes and contexts which aren't proper strings, usually close to the original strings but sometimes referencing something completely different.

Sample errors when the subscription is supposed to be 82623@default:

app_voicemail.c:3188 messagecount: Couldn't find mailbox  \MailboxExists in context xExists

app_voicemail.c:3188 messagecount: Couldn't find mailbox 3 in context default

app_voicemail.c:3188 messagecount: Couldn't find mailbox 82623 in context defaul0

app_voicemail.c:3188 messagecount: Couldn't find mailbox started at [  440]
taskprocessor.c ast_taskprocessor_get() in context  [  440]
taskprocessor.c ast_taskprocessor_get()

As a test, I test set the strings inside handle_subscribe to something constant, and all the subsequent queueing was fine:

static int handle_subscribe(void *datap)
{
 unsigned int len;
 struct mwi_sub *mwi_sub;
 struct mwi_sub_task *p = datap;

 p->mailbox="82623";
 p->context="default";
 ...

But if I set the mwist data right before ast_taskprocessor_push() is called in mwi_sub_event_cb(), the data is no longer reliable inside handle_subscribe():

 ...
 p->mailbox="82623";
 p->context="default";
 if (ast_taskprocessor_push(mwi_subscription_tps, handle_subscribe, mwist) < 0) {
   ast_free(mwist);
 }
}

So the task processor push process must be destroying the mwist struct data, or the data is being lost some how.  I couldn't pinpoint where or why, but not freeing mwist didn't solve the issue.


****** STEPS TO REPRODUCE ******

Allow subscriptions and turn on polling in voicemail.conf:
 pollmailboxes=yes
 pollfreq=30

Add subscriptions in sip.conf:
 allowsubscribe=yes

Wait till polling takes place.
The mailbox and context strings in handle_subscribe() will be broken, so eventually messagecount() will show an error.

Adding debug statements to handle_subscribe() will be simpler to display the strings, though.
Comments:By: Mark Michelson (mmichelson) 2008-05-23 13:31:28

I took a look at this and I have a hypothesis of what's happening wrong, but I'm not 100% certain. What I think is happening is not that the taskprocessor is changing the data, but that the copying of the strings into the mwist struct is not deep enough. What's happening is that instead of doing a deep copy of the strings, we instead do a shallow pointer assignment. As soon as mwi_sub_event_cb returns, the event system frees the event from which the information was taken, meaning that the taskprocessor now is operating using pointers to invalid memory. I suspect that a deeper copy in mwi_sub_event_cb will fix the issue. I will create a patch and upload it so that you may test it.
By: Mark Michelson (mmichelson) 2008-05-23 13:36:35

Try 12712.patch out and see if the problem persists. Thanks!
By: Tomo Takebe (tomo1657) 2008-05-23 14:26:24

Thanks putnopvut, the patch fixes the issue!
By: Digium Subversion (svnbot) 2008-05-23 14:51:34

Repository: asterisk
Revision: 118157

U   trunk/apps/app_voicemail.c

------------------------------------------------------------------------
r118157 | mmichelson | 2008-05-23 14:51:30 -0500 (Fri, 23 May 2008) | 10 lines

Use a deep copy on strings that come from ast_events. Otherwise it is
likely that after the event is freed, we no longer refer to valid memory.

(closes issue ASTERISK-12073)
Reported by: tomo1657
Patches:
     12712.patch uploaded by putnopvut (license 60)
Tested by: tomo1657


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=118157
By: Digium Subversion (svnbot) 2008-05-23 14:53:55

Repository: asterisk
Revision: 118158

_U  branches/1.6.0/

------------------------------------------------------------------------
r118158 | mmichelson | 2008-05-23 14:53:53 -0500 (Fri, 23 May 2008) | 17 lines

Blocked revisions 118157 via svnmerge

........
r118157 | mmichelson | 2008-05-23 14:57:40 -0500 (Fri, 23 May 2008) | 10 lines

Use a deep copy on strings that come from ast_events. Otherwise it is
likely that after the event is freed, we no longer refer to valid memory.

(closes issue ASTERISK-12073)
Reported by: tomo1657
Patches:
     12712.patch uploaded by putnopvut (license 60)
Tested by: tomo1657


........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=118158
By: Digium Subversion (svnbot) 2008-05-23 16:15:40

Repository: asterisk
Revision: 118162

_U  team/seanbright/resolve-shadow-warnings/
U   team/seanbright/resolve-shadow-warnings/CHANGES
U   team/seanbright/resolve-shadow-warnings/apps/app_chanisavail.c
U   team/seanbright/resolve-shadow-warnings/apps/app_voicemail.c
U   team/seanbright/resolve-shadow-warnings/channels/chan_gtalk.c
U   team/seanbright/resolve-shadow-warnings/channels/chan_usbradio.c
U   team/seanbright/resolve-shadow-warnings/channels/chan_vpb.cc
A   team/seanbright/resolve-shadow-warnings/configs/pbx_realtime.conf
A   team/seanbright/resolve-shadow-warnings/doc/cli.txt
U   team/seanbright/resolve-shadow-warnings/include/asterisk/utils.h
U   team/seanbright/resolve-shadow-warnings/main/utils.c
U   team/seanbright/resolve-shadow-warnings/pbx/pbx_realtime.c
U   team/seanbright/resolve-shadow-warnings/res/res_jabber.c
U   team/seanbright/resolve-shadow-warnings/res/res_odbc.c

------------------------------------------------------------------------
r118162 | seanbright | 2008-05-23 16:15:24 -0500 (Fri, 23 May 2008) | 127 lines

Merged revisions 117983,117986,117988,118020,118049,118053,118059,118101,118129,118157,118159 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r117983 | tilghman | 2008-05-22 17:27:00 -0400 (Thu, 22 May 2008) | 2 lines

Fix trunk breakage

................
r117986 | tilghman | 2008-05-22 17:42:50 -0400 (Thu, 22 May 2008) | 2 lines

Add a compatibility option for upgrading realtime extensions

................
r117988 | seanbright | 2008-05-22 17:43:54 -0400 (Thu, 22 May 2008) | 1 line

Split the compile flags out and wire up some dependencies
................
r118020 | phsultan | 2008-05-23 06:33:21 -0400 (Fri, 23 May 2008) | 15 lines

- remove whitespaces between tags in received XML packets before giving
them to the parser ;
- report Gtalk error messages from a buddy to the console.

This patch makes Asterisk "Google Jingle" (chan_gtalk) implementation
work with Empathy. Note that this is only true for audio streams, not
video.

Thank you to PH for his great help!

(closes issue ASTERISK-12017)
Reported by: PH
Patches:
     trunk-12647-1.diff uploaded by phsultan (license 73)
Tested by: phsultan, PH
................
r118049 | russell | 2008-05-23 08:37:31 -0400 (Fri, 23 May 2008) | 17 lines

Merged revisions 118048 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r118048 | russell | 2008-05-23 07:30:53 -0500 (Fri, 23 May 2008) | 9 lines

Don't declare a function that takes variable arguments as inline, because it's
not valid, and on some compilers, will emit a warning.

http://gcc.gnu.org/onlinedocs/gcc/Inline.html#Inline

(closes issue ASTERISK-11712)
Reported by: francesco_r
Patches by Tilghman, final patch by me

........

................
r118053 | tilghman | 2008-05-23 09:00:10 -0400 (Fri, 23 May 2008) | 11 lines

Merged revisions 118052 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r118052 | tilghman | 2008-05-23 07:59:16 -0500 (Fri, 23 May 2008) | 3 lines

Add information on using the Asterisk console, including tab command line
completion.  (Closes issue ASTERISK-12044)

........

................
r118059 | tilghman | 2008-05-23 09:20:13 -0400 (Fri, 23 May 2008) | 9 lines

Blocked revisions 118055 via svnmerge

........
r118055 | tilghman | 2008-05-23 08:18:44 -0500 (Fri, 23 May 2008) | 2 lines

Add format type checking for recently de-inlined function

........

................
r118101 | mvanbaak | 2008-05-23 13:12:04 -0400 (Fri, 23 May 2008) | 15 lines

add option 'a' to chanisavail.
If you give chanisavail a list of channels, it will only
return the first available channel.
When this option is set, it will return all the available
channels from the given list.

(closes issue ASTERISK-11673)
Reported by: dagmoller
Patches:
     app_chanisavail-snv.patch-v2.txt uploaded by dagmoller (license 436)
  - major changes by me because russellb pointed out some buffer overflows
    and codeguideline issues.
Converted it all to the ast_str_* api
Tested by: dagmoller, mvanbaak

................
r118129 | tilghman | 2008-05-23 14:09:14 -0400 (Fri, 23 May 2008) | 3 lines

Protect the object from changing while the 'odbc show' CLI command is running
(Closes issue ASTERISK-12065)

................
r118157 | mmichelson | 2008-05-23 15:57:40 -0400 (Fri, 23 May 2008) | 10 lines

Use a deep copy on strings that come from ast_events. Otherwise it is
likely that after the event is freed, we no longer refer to valid memory.

(closes issue ASTERISK-12073)
Reported by: tomo1657
Patches:
     12712.patch uploaded by putnopvut (license 60)
Tested by: tomo1657


................
r118159 | mmichelson | 2008-05-23 16:55:02 -0400 (Fri, 23 May 2008) | 4 lines

Get rid of warnings for those silly compilers which warn when freeing
a const pointer


................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=118162