[asterisk-dev] [Code Review] Lock the peer->mwipvt to avoid crashed in sip history management when overflowing history entries

[Russell Bryant]

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


The patch looks right to me in general (pending one minor change noted in a comment).

One other thing to be very careful with in a change like this is to make sure it is very clear what _other_ locks are held at this point in the code.  If locks are held, is locking the pvt like this the proper locking order?  It would be good to review all callers of this function to try to understand what locks are held coming in.  For example, is the sip_peer locked before calling this function?  This function doesn't appear to lock it, though it really should be based on what it's doing with sip_peer.  Locking preconditions/postconditions/assumptions should be documented in the doxygen comment above the function once someone figures them out.


./trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1373/#comment8027>

    The unlock should come before the unref.


- Russell


On Aug. 18, 2011, 6:45 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1373/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2011, 6:45 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Historically, mwi was sent by one thread (monitor) and did not need any locking. With the addition of event driven MWI, we now have two threads, the event thread and the monitor thread for retransmits, that work on the same pvt. We have had multiple crashes in 1.6.0 caused by fights over the sip history linked list and memory corruption when removing entries (when the list overflows). Looking at it, we realized that no locking was done so the two threads where allowed to have fun with the same pvt at the same time.
> 
> This affects everything since the historic day we released 1.6.0
> 
> 
> This addresses bug asterisk-18288.
>     https://issues.asterisk.org/jira/browse/asterisk-18288
> 
> 
> Diffs
> -----
> 
>   ./trunk/channels/chan_sip.c 332443 
> 
> Diff: https://reviewboard.asterisk.org/r/1373/diff
> 
> 
> Testing
> -------
> 
> Compiled fine. Felt fine. Under testing now.
> 
> 
> Thanks,
> 
> Olle E
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110818/a8de9961/attachment-0001.htm>