[asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel
rmudgett
reviewboard at asterisk.org
Fri Jan 17 17:50:54 CST 2014
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/#review10631
-----------------------------------------------------------
/branches/1.8/main/channel.c
<https://reviewboard.asterisk.org/r/3135/#comment20131>
This should not be done at all.
You are dropping a reference when timingdata doesn't really hold the reference. In the only case where timingdata is an identified ao2 object, the timingdata pointer is sharing the reference with c->stream. The reference is dropped by ast_closestream() after clearing the timingdata with its own call to ast_settimeout(). Otherwise, you will need to give timingdata a reference when setting an identified ao2 object.
/branches/1.8/main/channel.c
<https://reviewboard.asterisk.org/r/3135/#comment20130>
Setting timingdata to NULL here is unnecessary since it is set with a new value right after. It is the purpose of the function.
- rmudgett
On Jan. 17, 2014, 4:18 p.m., Russell Bryant wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3135/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 4:18 p.m.)
>
>
> Review request for Asterisk Developers and leifmadsen.
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> The ast_filestream object gets tacked on to a channel via
> chan->timingdata. It's a reference counted object, but the reference
> count isn't used when putting it on a channel. It's theoretically
> possible for another thread to interfere with the channel while it's
> unlocked and cause the filestream to get destroyed.
>
> Use the astobj2 reference count to make sure that as long as this code
> path is holding on the ast_filestream and passing it into the file.c
> playback code, that it knows it's valid.
>
> -----
>
> More detail:
>
> A crash occurs in voicemail. Here is the backtrace:
>
> #0 0x00000000004d396a in ast_readaudio_callback (s=0x7f260856f580) at file.c:779
> #1 0x00000000004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
> #2 0x000000000047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at channel.c:3865
> #3 0x0000000000477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
> #4 0x00000000004d5231 in waitstream_core (c=0x7f2644c49460, breakon=0x7f262ed56f00 "#*", forward=0x64c4a2 "", reverse=0x64c4a2 "", skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
> #5 0x00000000004d5783 in ast_waitstream (c=0x7f2644c49460, breakon=0x7f262ed56f00 "#*") at file.c:1344
> #6 0x00007f266d34c22b in leave_voicemail (chan=0x7f2644c49460, ext=0x7f2609908c20 "<redacted>", options=0x7f262ed56ff0) at app_voicemail.c:5773
> #7 0x00007f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 "<redacted>") at app_voicemail.c:10722
>
>
> (gdb) frame 0
> #0 0x00000000004d396a in ast_readaudio_callback (s=0x7f260856f580) at file.c:779
> 779 if (s->owner->timingfd > -1) {
> (gdb) p s
> $6 = (struct ast_filestream *) 0x7f260856f580
>
> The crash occurs here because the contents of the ast_filestream are garbage.
>
> (gdb) p *s
> $7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769, open_filename = 0x440a0d3330393734 <Address 0x440a0d3330393734 out of bounds>, filename = 0x6974616e69747365 <Address 0x6974616e69747365 out of bounds>,
> realfilename = 0x440a0d73203a6e6f <Address 0x440a0d73203a6e6f out of bounds>, vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr = 0x2d7473786e203a74, lastwriteformat = 762475363, lasttimeout = 1969583473,
> owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype = 875836727, subclass = {integer = 926687266, codec = 3761973748257529890}, datalen = 809056052, samples = 168640051, mallocd = 1851877443,
> mallocd_hdr_len = 7881689877736674080, offset = 762148397, src = 0xd61633832313030 <Address 0xd61633832313030 out of bounds>, data = {ptr = 0x616e69747365440a, uint32 = 1936016394, pad = "\nDestina"}, delivery = {
> tv_sec = 7953753055737899380, tv_usec = 5785246594218354030}, frame_list = {next = 0x36702d5453584e2f}, flags = 758134073, ts = 7010989772577650738, len = 7163375912484959347, seqno = 1869182049},
> buf = 0x614c0a0d65756575 <Address 0x614c0a0d65756575 out of bounds>, _private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e <Address 0x7273632d7473786e out of bounds>,
> write_buffer = 0x38312c2c2c2c712d <Address 0x38312c2c2c2c712d out of bounds>}
> (gdb) p s->owner
> $8 = (struct ast_channel *) 0x656c6c61430a0d65
> (gdb) p *s->owner
> Cannot access memory at address 0x656c6c61430a0d65
>
> s->owner should have been 0x7f2644c49460, from higher up in the backtrace.
>
>
> Here is where things get quite interesting ...
>
>
> (gdb) frame 2
> #2 0x000000000047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at channel.c:3865
> 3865 func(data);
> (gdb) list
> 3860 /* save a copy of func/data before unlocking the channel */
> 3861 int (*func)(const void *) = chan->timingfunc;
> 3862 void *data = chan->timingdata;
> 3863 chan->fdno = -1;
> 3864 ast_channel_unlock(chan);
> 3865 func(data);
> 3866 } else {
> 3867 ast_timer_set_rate(chan->timer, 0);
> 3868 chan->fdno = -1;
> 3869 ast_channel_unlock(chan);
> (gdb) p chan->timingfunc
> $9 = (int (*)(const void *)) 0
> (gdb) p chan->timingdata
> $10 = (void *) 0x0
> (gdb) p func
> $11 = (int (*)(const void *)) 0x4d3b6a <ast_fsread_audio>
> (gdb) p data
> $12 = (void *) 0x7f260856f580
>
>
> This is the code inside of ast_read() that executes the callback ast_fsread_audio() from main/file.c to play the next bits of the audio file to the channel. We can see that chan->timingfunc and chan->timingdata have now been reset since this code ran. That probably also means that the ast_filestream got destroyed before the code in ast_readaudio_callback was done using it.
>
> The only way I can think of this happening is via something in another thread. Something like AMI running on a channel doing something that affects audio. I'm basically speculating at this point. If this is the case, it seems like the filestream's reference count needs to be bumped while it's on the channel. Otherwise, it seems possible that it could disappear at just the wrong time.
>
>
> Diffs
> -----
>
> /branches/1.8/main/file.c 405876
> /branches/1.8/main/channel.c 405876
> /branches/1.8/include/asterisk/channel.h 405876
>
> Diff: https://reviewboard.asterisk.org/r/3135/diff/
>
>
> Testing
> -------
>
> This issue was originally reported by Leif Madsen. The patch was given to him for further testing. They have done some basic sanity tests with it, but it's not yet running in production or anything like that. It at least doesn't blow up immediately ...
>
> I wanted to get the patch and analysis up to get some more eyes on it.
>
>
> Thanks,
>
> Russell Bryant
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140117/aab71a86/attachment-0001.html>
More information about the asterisk-dev
mailing list