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

[wdoekes]

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


>> Making this if(ssl) only is a regression, I think.

> The check about authenticated still occurs at the beginning of the for loop
> in _sip_tcp_helper_thread(), so it can still occur for TCP sockets.
> What I was seeing was that this was checked as data was read on the socket
> when using a FILE handle. When using recv() directly, there are no
> incremental places where I can do such a check. I've added it after
> ast_wait_for_input() just to be safe, though. With the content-length
> checking stuff, it could actually be triggered.

What do you mean, no incremental places? You just added the while loop where we

  res = ast_wait_for_input(tcptls_session->fd, -1);

wait indefinitely for packets. Or?


The other problem that you didn't address was reading too *much* data.

In the SSL version, fgets() is used for the loop-over-the-headers bit.
Although superugly, this does make sure that you won't read too much.

In the new tcp case, a 100 with an immediate 180 following it could
be combined into a single read: now we kill the connection because we've
"read too many bytes".


The obvious fix would be to store the leftover bytes in a buffer and use
those on the next iteration.




/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/2123/#comment13865>

    Should be (sizeof(readbuf) - 1).
    
    I'd make readbuf 4097 bytes long. Should be sufficient, and the loop takes care of the rest.



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/2123/#comment13866>

    Perhaps a return -1 if someone tries to be funny and sends a negative content_length.


- wdoekes


On Sept. 25, 2012, 1:25 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2123/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2012, 1:25 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 373633 
> 
> 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/20121002/fc9dd3f2/attachment-0001.htm>