[asterisk-dev] [Code Review] 3964: CDRs: Fix crash caused by infinite generation of new CDR records when a channel enters, leaves, and enters a multi-party bridge

rmudgett reviewboard at asterisk.org
Wed Sep 3 14:08:18 CDT 2014


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



/branches/12/main/cdr.c
<https://reviewboard.asterisk.org/r/3964/#comment23706>

    * Nothing cares about this return value.
    
    * The RAII_VAR and SCOPED_AO2LOCK can easily be eliminated by changing the "return 0" statements in the for loop to breaks.



/branches/12/main/cdr.c
<https://reviewboard.asterisk.org/r/3964/#comment23705>

    This change has no effect.  The break only exited the switch statement and then control goes to the next iteration in the loop.  Changing it to a continue just does the same thing.



/branches/12/main/cdr.c
<https://reviewboard.asterisk.org/r/3964/#comment23707>

    This is a tail recursion that could be eliminated by pulling the above for loop into its own routine.  Then the cdr lock also won't be recursively obtained.


- rmudgett


On Sept. 1, 2014, 11:51 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3964/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2014, 11:51 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24241
>     https://issues.asterisk.org/jira/browse/ASTERISK-24241
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes an issue where CDRs would get stuck generating an infinite number of CDRs, eventually crashing Asterisk.
> 
> When a channel enters into a multi-party bridge, the CDR engine creates mappings of each participant to each other participant, picking the 'A' party as it goes. So, if we have four channels in a multi-party bridge (Alice, Bob, Charlie, Denise), we would have something like:
> 
> Alice => Bob
> Alice => Charlie
> Alice => Denise
> Bob => Charlie
> Bob => Denise
> Charlie => Denise
> 
> This works fine when participants enter the bridge a single time.
> 
> When a participant leaves a bridge, the CDRs for that channel are transitioned to a finalized state. In the previous example, if Bob leaves the bridge, then the following CDRs are all marked as finalized:
> 
> Alice => Bob (F)
> Bob => Charlie (F)
> Bob => Denise (F)
> 
> The bug occurs if Bob rejoins. When the CDR engine creates mappings between the channels, a CDR in the finalized state reports back to the engine "nope, I can't handle this, you need a new CDR". That is correct, but the code that generates the mappings makes two mistakes at that point:
> (1) It bails instead of looking for more CDRs, immediately making a new CDR and appending it to the chain. When you make a new CDR for a channel, it does not originally have a Party B - so the engine calls the bridge mapping code "knowing" that Bob will be a perfect Party B.
> (2) When a new CDR is appended to a chain, one code path allowed us to continue on down the chain inspecting more CDRs. We do update the 'new' CDR, but we end up transitioning it to the Bridge state and calling the original code on it again, hitting point #1 again (as there is still an original CDR in the finalized state).
> 
> Let's say Bob enters the bridge again. The CDR engine starts making pairs of CDRs, starting with Alice. Since Alice and Bob's original CDR is finalized (he left the bridge), the record returns "you need a new one". The CDR engine makes a new CDR for Alice, appends it to her CDRs, and then tells Bob to go see if he can find a CDR for Alice that is suitable. This hits bug #2, which maps Bob to Alice but then keeps looping instead of returning automatically. When we're done updating the new CDR, the original code takes over, hits point #1 again, and generates a new CDR. Hilarity!
> 
> We end up with:
> 
> Alice => Bob (F)   <--- this keeps getting inspected and telling us to generate new CDRs :-(
> Alice => Bob (new)
> Alice => Bob (new)
> Alice => Bob (new)
>  ...
> 
> This patch fixes the two glitches mentioned above.
> (1) If, when making matches, a CDR returns 'you need a new one', we keep inspecting the chain to see if anyone else can take a match for us
> (2) When a match is made, we return if we successfully placed our channel as the Party B on an existing CDR, instead of continuing to match up
> 
> And, even after this bug, I still maintain this is easier to diagnose and fix then the original CDR code in features.c :-)  If nothing else, all of this code was in one place, and I didn't have to think "what happens if something masquerades right now".
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/cdr.c 422502 
> 
> Diff: https://reviewboard.asterisk.org/r/3964/diff/
> 
> 
> Testing
> -------
> 
> All existing unit tests/testsuite tests pass.
> 
> Issue was reproduced using the test on https://reviewboard.asterisk.org/r/3965. The test creates 5 Local channels, puts them in a multi-party bridges, then removes channels 0 and 3 and re-adds them.
> 
> Without this patch - explosions. With this patch - we generate the correct number of CDRs even when channels bounce themselves in and out of multi-party bridges.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140903/3d014cd6/attachment.html>


More information about the asterisk-dev mailing list