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

[jrose]

> On Nov. 21, 2011, 3:19 a.m., wdoekes wrote:
> > Better. A couple of points:
> 
> wdoekes wrote:
>     P.S. A couple of relevant bugs:
>     
>     https://issues.asterisk.org/jira/browse/ASTERISK-18342
>     https://issues.asterisk.org/jira/browse/ASTERISK-18345
>     
>     I linked them in Jira.

I'll keep this in mind for the final log message.  Definitely seem related, and hopefully they might be fixed by these upcoming patches.


> On Nov. 21, 2011, 3:19 a.m., wdoekes wrote:
> > /trunk/channels/chan_sip.c, line 25957
> > <https://reviewboard.asterisk.org/r/1576/diff/3/?file=21782#file21782line25957>
> >
> >     I prefer:
> >     
> >     s->fd = -1;
> >     
> >     Otherwise you have to look inside the opaque ast_tcptls_close_session_file to know what fd is. (And.. as an added bonus, setting it from a constant is cheaper.)
> >     
> >     Unless you plan to *not* set tcptls_session->fd to -1 under some circumstances, but that would likely only add to the confusion.

Right.  The tcptls_session->fd should always be set to -1 by ast_tcptls_close_session_file unless it already is -1, so that's a safe assumption and this change makes perfect sense.


> On Nov. 21, 2011, 3:19 a.m., wdoekes wrote:
> > /trunk/main/tcptls.c, lines 79-94
> > <https://reviewboard.asterisk.org/r/1576/diff/3/?file=21784#file21784line79>
> >
> >     I would go with something like this:
> >     
> >     static int ssl_close(void *cookie)
> >     {
> >       int cookie_fd = SSL_get_fd(cookie);
> >       int ret;
> >       if (cookie_fd > -1) {
> >         /* According to the TLS standard, it is acceptable for an application to only send its shutdown alert and then close the underlying connection without waiting for the peer's response (this way resources can be saved, as the process can already terminate or serve another connection). */
> >         if ((ret = SSL_shutdown(cookie)) < 0) {
> >           ast_log(LOG_ERROR, "SSL_shutdown() failed: %d\n", SSL_get_error(cookie, ret));
> >         }
> >         SSL_free(cookie);
> >         /* adding shutdown(2) here has no added enefit */
> >         if (close(cookie_fd)) {
> >           ast_log(LOG_ERROR, "close() failed: %s\n", strerror(errno));
> >         }
> >       }
> >       return 0;
> >     }
> >     
> >     But -- and a big one: doing things by the book here (the new SSL_shutdown) MAY case the thread to block during this operation.
> >     
> >     If it turns out that moving up the SSL_shutdown causes hangs, it should probably be removed altogether (with a nice #if 0) until a better solution is implemented.

Well, I changed this in as you have suggested and in a little rudimentary test it seems to work fine under normal conditions so far with the SSL_close occuring once the SIP dialog expires... no particularly noticeable problems anyway, as it got through the code path all at once, but it was just a basic SIP TLS call test.  I'll keep an eye open for any reports related to this if we can commit it.

Getting ready to post the revised patch.


> On Nov. 21, 2011, 3:19 a.m., wdoekes wrote:
> > /trunk/include/asterisk/tcptls.h, line 180
> > <https://reviewboard.asterisk.org/r/1576/diff/3/?file=21783#file21783line180>
> >
> >     + and sets them to NULL and -1 respectively.

k, got it.


- jrose


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


On Nov. 17, 2011, 11:01 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1576/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2011, 11:01 a.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 344846 
>   /trunk/include/asterisk/tcptls.h 344846 
>   /trunk/main/tcptls.c 344846 
> 
> 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/20111130/7bf0e2da/attachment-0001.htm>