Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[Russell Bryant]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---

(Updated Jan. 27, 2014, 1:25 a.m.)


Status
--

This change has been marked as submitted.


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  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
file.c:779
#1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
#3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4  0x004d5231 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  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
breakon=0x7f262ed56f00 "#*") at file.c:1344
#6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at app_voicemail.c:5773
#7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
"") at app_voicemail.c:10722


(gdb)  frame 0
#0  0x004d396a 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 , 
filename = 0x6974616e69747365 , 
  realfilename = 0x440a0d73203a6e6f , 
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 , 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 , _private 
= 0x203a617461447473, orig_chan_name = 0x7273632d7473786e , 
  write_buffer = 0x38312c2c2c2c712d }
(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  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
3865func(data);
(gdb) list
3860/* save a copy of func/data before 
unlocking the channel */
3861int (*func)(const void *) = 
chan->timingfunc;
3862void *data = chan->timingdata;
3863chan->fdno = -1;
3864ast_channel_unlock(chan);
3865func(data);
3866} else {
3867ast_timer_set_rate(chan->timer, 0);
3868chan->fdno = -1;
3869ast_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 
(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 A

Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/#review10638
---

Ship it!


Looks good to me.

- rmudgett


On Jan. 17, 2014, 6:58 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, 6:58 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  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
> file.c:779
> #1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
> #2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> #3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
> #4  0x004d5231 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  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
> breakon=0x7f262ed56f00 "#*") at file.c:1344
> #6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
> ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at 
> app_voicemail.c:5773
> #7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
> "") at app_voicemail.c:10722
> 
> 
> (gdb)  frame 0
> #0  0x004d396a 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  bounds>, filename = 0x6974616e69747365  bounds>, 
>   realfilename = 0x440a0d73203a6e6f  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 , 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 , 
> _private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e  0x7273632d7473786e out of bounds>, 
>   write_buffer = 0x38312c2c2c2c712d  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  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> 3865func(data);
> (gdb) list
> 3860/* save a copy of func/data before 
> unlocking the channel */
> 3861int (*func)(const void *) = 
> chan->timingfunc;
> 3862void *data = chan->timingdata;
> 3863chan->fdno = -1;
> 3864ast_channel_unlock(chan);
> 3865func(data);
> 3866} else {
> 3867ast_timer_set_rate(chan->timer, 0);
> 3868chan->fdno = -1;
> 3869ast_channel_unlock(chan);
> (gdb) p chan->timingfunc
> $9 = (int (*)(const void *)) 0
> (gdb) p chan->timingdata
> $10 = (void *) 0x0
> (gdb) p func
> $11 = (

Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[Russell Bryant]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---

(Updated Jan. 18, 2014, 12:58 a.m.)


Review request for Asterisk Developers and leifmadsen.


Changes
---

Addressed Richard's comments.

Thanks for the review!


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  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
file.c:779
#1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
#3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4  0x004d5231 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  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
breakon=0x7f262ed56f00 "#*") at file.c:1344
#6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at app_voicemail.c:5773
#7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
"") at app_voicemail.c:10722


(gdb)  frame 0
#0  0x004d396a 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 , 
filename = 0x6974616e69747365 , 
  realfilename = 0x440a0d73203a6e6f , 
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 , 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 , _private 
= 0x203a617461447473, orig_chan_name = 0x7273632d7473786e , 
  write_buffer = 0x38312c2c2c2c712d }
(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  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
3865func(data);
(gdb) list
3860/* save a copy of func/data before 
unlocking the channel */
3861int (*func)(const void *) = 
chan->timingfunc;
3862void *data = chan->timingdata;
3863chan->fdno = -1;
3864ast_channel_unlock(chan);
3865func(data);
3866} else {
3867ast_timer_set_rate(chan->timer, 0);
3868chan->fdno = -1;
3869ast_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 
(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.  
S

Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[Russell Bryant]

> On Jan. 17, 2014, 11:50 p.m., rmudgett wrote:
> > /branches/1.8/main/channel.c, lines 3576-3579
> > 
> >
> > 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  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
> file.c:779
> #1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
> #2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> #3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
> #4  0x004d5231 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  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
> breakon=0x7f262ed56f00 "#*") at file.c:1344
> #6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
> ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at 
> app_voicemail.c:5773
> #7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
> "") at app_voicemail.c:10722
> 
> 
> (gdb)  frame 0
> #0  0x004d396a 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  bounds>, filename = 0x6974616e69747365  bounds>, 
>   realfilename = 0x440a0d73203a6e6f  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 , 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 , 
> _private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e  0x7273632d7473786e out of bounds>, 
>   write_buffer = 0x38312c2c2c2c712d  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  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> 3865func(data);
> (gdb) list
> 3860/

Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/#review10631
---



/branches/1.8/main/channel.c


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


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  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
> file.c:779
> #1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
> #2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> #3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
> #4  0x004d5231 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  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
> breakon=0x7f262ed56f00 "#*") at file.c:1344
> #6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
> ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at 
> app_voicemail.c:5773
> #7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
> "") at app_voicemail.c:10722
> 
> 
> (gdb)  frame 0
> #0  0x004d396a 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  bounds>, filename = 0x6974616e69747365  bounds>, 
>   realfilename = 0x440a0d73203a6e6f  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 , 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 , 
> _private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e  0x7273632d7473786e out of bounds>, 
>   write_buffer = 0x38312c2c2c2c712d  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  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> 3865func(data);
> (gdb) list
> 3860/* save a copy of func/data before 

[asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[Russell Bryant]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---

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  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
file.c:779
#1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
#3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4  0x004d5231 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  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
breakon=0x7f262ed56f00 "#*") at file.c:1344
#6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at app_voicemail.c:5773
#7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
"") at app_voicemail.c:10722


(gdb)  frame 0
#0  0x004d396a 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 , 
filename = 0x6974616e69747365 , 
  realfilename = 0x440a0d73203a6e6f , 
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 , 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 , _private 
= 0x203a617461447473, orig_chan_name = 0x7273632d7473786e , 
  write_buffer = 0x38312c2c2c2c712d }
(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  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
3865func(data);
(gdb) list
3860/* save a copy of func/data before 
unlocking the channel */
3861int (*func)(const void *) = 
chan->timingfunc;
3862void *data = chan->timingdata;
3863chan->fdno = -1;
3864ast_channel_unlock(chan);
3865func(data);
3866} else {
3867ast_timer_set_rate(chan->timer, 0);
3868chan->fdno = -1;
3869ast_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 
(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