[asterisk-dev] [Code Review]: Eliminate redundant and possibly dangerous close/fclose pairs in chan_sip and tcptls.

[jrose]

> On Nov. 11, 2011, 2:36 p.m., wdoekes wrote:
> > /trunk/main/tcptls.c, lines 79-85
> > <https://reviewboard.asterisk.org/r/1576/diff/2/?file=21710#file21710line79>
> >
> >     That fclose() closes the specific fd is not because fclose is so cool, but because of the code here.
> >     
> >     However: this looks broken. Calling SSL_shutdown after closing the socket seems wrong. (There's a whole bit about calling SSL_shutdown twice here: http://www.openssl.org/docs/ssl/SSL_shutdown.html but I don't know how much applies here.)

So you think we should attempt a double shutdown of SSL_shutdown, then perform the close operation? I hope I interpreted this correctly.

I should probably also check to make sure SSL_get_fd(cookie) doesn't return -1 before attempting to close it as well.


> On Nov. 11, 2011, 2:36 p.m., wdoekes wrote:
> > /trunk/channels/chan_sip.c, lines 2815-2822
> > <https://reviewboard.asterisk.org/r/1576/diff/2/?file=21708#file21708line2815>
> >
> >     Again -- if you're reading upward, like I'm writing this review -- I'd rather have someone prove that fd is never true when f is NULL.

Not entirely sure how I attempted to follow this review (a lot of this stuff I'm pretty inexperienced in really), but if we remove all of the things that would otherwise set f to NULL and call them through ast_tcptls_close_session_file, then that should be true all the time since that's going to set the fd to -1 every time it closes the file stream.


> On Nov. 11, 2011, 2:36 p.m., wdoekes wrote:
> > /trunk/main/tcptls.c, lines 148-150
> > <https://reviewboard.asterisk.org/r/1576/diff/2/?file=21710#file21710line148>
> >
> >     This bit is ok. Although I would add a tcptls_session->fd = -1 here.
> 
> jrose wrote:
>     Wow, you are absolutely correct.  I can't believe I forgot to do that... it was on my whiteboard and even in my revision 1 :<.

Actually, I wonder if there is any reason not to just go ahead and use the new ast_tcptls_close_session_file here... since fclose closes the file descriptor anyway, I can't imagine there really being any reason not to... and it'll also check to make sure it's safe, handle all the error messages, set fd to -1 and f to Null... all that business.


> On Nov. 11, 2011, 2:36 p.m., wdoekes wrote:
> > /trunk/main/tcptls.c, line 217
> > <https://reviewboard.asterisk.org/r/1576/diff/2/?file=21710#file21710line217>
> >
> >     You're right. The return value of fopencookie/funopen is not checked, so we could indeed have an unclosed ->fd here.
> >     
> >     Personally, I would've preferred !..->f check a bit higher up, and only the fclose() here.
> >     
> >     Right now it becomes more and more of a guess when which variable is what.

That's really not something I'm correct about... (the existing code was already attempting to close it) I was just making sure that if the fd was already closed that our attempt to close it would check for that before attempting to close it.  As far as I know, attempting to close(-1) probably isn't a good idea, though at the same time, the manpages don't explicitly say doing so is dangerous.  


- jrose


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


On Nov. 11, 2011, 2:06 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1576/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2011, 2:06 p.m.)
> 
> 
> Review request for Asterisk Developers, David Vossel and mjordan.
> 
> 
> Summary
> -------
> 
> According to the reporter, chan_sip and tcptls were using an odd combination of close and fclose which can result in undefined behavior.  Following the man pages for fclose and close, attempting to fclose a file with an already closed file descriptor results in undefined behavior, and fclose itself will already close the file descriptor, so using fclose and then close is redundant.  The reporter suggested as well that since file descriptors will experience frequent reuse that using fclose and then close could also result in closing a file descriptor that is in use elsewhere since there is time for that that file descriptor index to be reclaimed in the file descriptor table during the window between that fclose and the following close operation... or at least that's how I interpreted it.
> 
> I removed all uses of the close function when there was an adjacent fclose.  I'm still not 100% sure if this is the right approach since this behavior introduced in a patch by dvossel in r225445, which can be seen here: http://lists.digium.com/pipermail/asterisk-commits/2009-October/038031.html  I'm a little worried that this might be because the file descriptor received a redundant reference somewhere along the line and these close() functions might have been used to close a file descriptor leak or something along those lines.
> 
> 
> This addresses bug ASTERISK-18700.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18700
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 344329 
>   /trunk/include/asterisk/tcptls.h 344329 
>   /trunk/main/tcptls.c 344329 
> 
> Diff: https://reviewboard.asterisk.org/r/1576/diff
> 
> 
> Testing
> -------
> 
> Set up some TLS calls and used core show fd (with DEBUG_FD_LEAKS enabled) to make sure this wasn't causing a bunch of file descriptor leaks.  From what I could find, it wasn't.
> 
> 
> Thanks,
> 
> jrose
> 
>

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