<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Russell Bryant escribió:
<blockquote
cite="mid:20090915142740.3947.98938@hotblack.digium.internal"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">On 2009-09-15 09:15:28, Tilghman Lesher wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Looks good! BTW, make sure that you document this change in CHANGES, because this will break some people's (poor) designs to access channel variables in CDR code.
</pre>
</blockquote>
</blockquote>
<pre wrap=""><!---->
I'm not sure that I understand what you mean with regards to changes in CDR behaviour. Can you please expand upon your comments to explain how this will change behaviour of CDR processing?
</pre>
</blockquote>
Well, I think that the "poor" designs Russell brought to mind, could be
some that on step 6 (do CDR stuff) make use of channel variables to
store extra information in the CDR code. With the change you made, this
information could be not available, but I'm not sure because the last
step is what you say that finally clears the channel variables.<br>
<br>
<blockquote
cite="mid:20090915142740.3947.98938@hotblack.digium.internal"
type="cite">
<pre wrap="">- Russell
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
<a class="moz-txt-link-freetext" href="https://reviewboard.asterisk.org/r/362/#review1068">https://reviewboard.asterisk.org/r/362/#review1068</a>
-----------------------------------------------------------
On 2009-09-15 08:57:01, Matthew Nicholson wrote:
</pre>
<blockquote type="cite">
<pre wrap="">-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
<a class="moz-txt-link-freetext" href="https://reviewboard.asterisk.org/r/362/">https://reviewboard.asterisk.org/r/362/</a>
-----------------------------------------------------------
(Updated 2009-09-15 08:57:01)
Review request for Asterisk Developers.
Summary
-------
This patch fixes a race condition that can occur if a channel is retrieved from the channel list while it is being hung up. This situation can be caused if the pbx thread calls ast_hangup on a channel while another thread attempts to retrieve the channel from the channel list. One example where this occurs involves the 'Bridge' manager action.
An approximation of the hangup procedure in ast_hangup() follows:
1. lock the channel
2. free some fields in the channel
3. call chan->tech->hangup()
4. unlock the channel
5. generate the 'Hangup' manager event
6. do CDR stuff
7. remove the channel from the channel list
8. lock and unlock the channel
9. free the remaining channel fields
</pre>
</blockquote>
</blockquote>
Thank you very much for this fix. I'm sure that this will greatly
improve asterisk stability, especially on very busy servers that
incessantly make queries about status of channels.<br>
<blockquote
cite="mid:20090915142740.3947.98938@hotblack.digium.internal"
type="cite">
<blockquote type="cite">
<pre wrap="">
During step 3, the channel tech_pvt field will be freed and set to NULL. After step 4 and before step 7, the channel may be retrieved from the channel list and used. Problems can occur if any of the freed fields are used. In this specific instance the tech_pvt field is dereferenced causing asterisk to crash. My purposal for fixing the problem involves moving step 7 and inserting it before step 3.
This problem potentially exists in all supported versions of asterisk.
This addresses bug 15316.
<a class="moz-txt-link-freetext" href="https://issues.asterisk.org/view.php?id=15316">https://issues.asterisk.org/view.php?id=15316</a>
Diffs
-----
/tags/1.6.1.1/main/channel.c 218291
</pre>
</blockquote>
</blockquote>
<blockquote
cite="mid:20090915142740.3947.98938@hotblack.digium.internal"
type="cite">
<blockquote type="cite">
<pre wrap="">
Diff: <a class="moz-txt-link-freetext" href="https://reviewboard.asterisk.org/r/362/diff">https://reviewboard.asterisk.org/r/362/diff</a>
Testing
-------
To test this, I replicated the customer's environment from bug 15316 (this involves sending a Bridge manager action at the proper time). In order to achieve the proper timing for the race condition to manifest itself, I added a sleep() command after step 3 in the procedure above and a shorter sleep withing the Bridge action (action_bridge()) just before retrieving a channel from the channel list. While the Bridge action thread is sleeping, I hang up the channel requested for the bridge action causing the hangup procedure to run through step 3 then sleep. The Bridge action thread then wakes and retrieves the channel from the channel list while the hangup thread is sleeping. The Bridge thread now has a partially torn down channel with a NULL tech_pvt. Eventually the Bridge thread executes ast_rtp_new_source() which attempts to access the tech_pvt and crashes asterisk.
After moving step 7 and inserting it before step 3, the crash no longer occurs and the Bridge thread never finds the channel in the channel list.
Thanks,
Matthew
</pre>
</blockquote>
</blockquote>
<pre class="moz-signature" cols="72">--
Ing. Miguel Molina
Grupo de Tecnología
Millenium Phone Center
</pre>
</body>
</html>