[asterisk-dev] [Code Review]: Do not use FILE handles when doing SIP TCP reads

[Mark Michelson]

> On Sept. 20, 2012, 12:08 p.m., Paul Belanger wrote:
> > /branches/1.8/channels/chan_sip.c, lines 2511-2512
> > <https://reviewboard.asterisk.org/r/2123/diff/1/?file=31312#file31312line2511>
> >
> >     add ast_debug() explaining the error?  Other checks seem to do that

After looking more closely, the reason no message is printed here is because sip_check_authtimeout() already prints an error message if it is going to return less than 0.


> On Sept. 20, 2012, 12:08 p.m., Paul Belanger wrote:
> > /branches/1.8/channels/chan_sip.c, lines 2500-2501
> > <https://reviewboard.asterisk.org/r/2123/diff/1/?file=31312#file31312line2500>
> >
> >     Doxgen comments if you can.  Since we have a new function

Fixed.


> On Sept. 20, 2012, 12:08 p.m., Paul Belanger wrote:
> > /branches/1.8/channels/chan_sip.c, line 2615
> > <https://reviewboard.asterisk.org/r/2123/diff/1/?file=31312#file31312line2615>
> >
> >     same, doxygen comment.

Fixed.


> On Sept. 20, 2012, 12:08 p.m., Paul Belanger wrote:
> > /branches/1.8/channels/chan_sip.c, lines 2563-2588
> > <https://reviewboard.asterisk.org/r/2123/diff/1/?file=31312#file31312line2563>
> >
> >     Is it me, or is this code duplicated from above? Perhaps moving it into a common function?

While I certainly share the sentiment that repeating yourself is bad form, I couldn't find an elegant way to just isolate these bits of logic without making things more awkward than they already are.

The two if statements are separate logical operations, so ideally I'd write two functions. The problem is that each of these can potentially set state information and/or return due to error conditions. It's certainly possible to write two functions that can return different conditions and set output parameters, but it's just...gross.

This function in general could do with a larger makeover. The polling logic is just plain wonky. Since the TLS function was not really what I changed (I just copied it out to a separate function from where it was before), I'm going to leave this alone for now since messing with it is likely more dangerous than leaving it as it is.


- Mark


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


On Sept. 20, 2012, 2:41 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2123/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2012, 2:41 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The reporter of issue ASTERISK-20213 had an issue where Asterisk would lock up after being used for a few days. When looking at backtraces, it was apparent that the problematic thread was the SIP TCP thread. It was blocked in a call to fgets(). This blocked thread was holding a lock that the SIP monitor thread was trying to lock. Once the SIP monitor thread was stuck trying to grab the lock, it meant that no SIP traffic could be received.
> 
> While the reason why the fgets() call blocked was not ever made explicitly clear, it certainly seemed odd that a successful poll() would result in an fgets() that would block forever. The obvious oddness was that we were polling on a file descriptor but then trying to read from a corresponding FILE handle. This, in the general opinion of everyone, is "stupid". I supplied a patch to the reporter that uses recv() instead of fgets() for TCP SIP connections, hoping this would work.
> 
> As it turns out, the patch has been in use for over three weeks with no issues, so it appears to be a good fix. The patch specifically targets TCP connections and not TLS. TLS connections were not reported as having the issue, plus changing TLS would be a much more invasive operation.
> 
> In my opinion, we should remove the use of FILE handles altogether in the TCP/TLS code, but such a task would be better suited for Asterisk trunk instead of a released version. For now, fixing the problems as they are reported is the best option.
> 
> Note that the reporter reported his issue against Asterisk 10 but this review is made against Asterisk 1.8. This is because the same method of retrieving TCP data is used in 1.8 so I believe the issue must exist there as well.
> 
> While viewing my changes, pay particular attention to the TLS code to ensure I did not introduce any subtle logic changes. The sip_tls_read() function is pretty much a copy and paste of the code that existed before, so I am hopeful that I have not introduced anything undesirable there.
> 
> 
> This addresses bug ASTERISK-20212.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20212
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 372950 
> 
> Diff: https://reviewboard.asterisk.org/r/2123/diff
> 
> 
> Testing
> -------
> 
> In the reporter's words:
> 
> "we have had the first patch in since my last comment with ZERO failures. I think at this point it is safe to say that fix will work (and is working)."
> 
> 
> Thanks,
> 
> Mark
> 
>

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