> On Jan. 17, 2014, 11:50 p.m., rmudgett wrote: > > /branches/1.8/main/channel.c, lines 3576-3579 > > <https://reviewboard.asterisk.org/r/3135/diff/1/?file=52962#file52962line3576> > > > > 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.
I intended to have a corresponding ao2_ref(obj, 1) in ast_settimeout() when it gets stored on the channel. I'll fix that. - Russell ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3135/#review10631 ----------------------------------------------------------- On Jan. 17, 2014, 10: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, 10: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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
