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

[jrose]

> On Nov. 11, 2011, 2:13 p.m., Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, lines 2817-2819
> > <https://reviewboard.asterisk.org/r/1576/diff/1/?file=21684#file21684line2817>
> >
> >     One thing to do when trapping this fclose() is check if errno is something like EINTR or EAGAIN and retry the operation.  This is possible, for example, if the underlying syscall is interrupted by a signal.  The danger of not checking for these values is a possible file descriptor leak.
> 
> wdoekes wrote:
>     Not according to my fclose manual:
>     
>     RETURN VALUE
>            Upon successful completion 0 is returned.  Otherwise, EOF is returned and
>            the global variable errno is set to indicate the error.  In  either  case
>            any  further  access  (including  another call to fclose()) to the stream
>            results in undefined behavior.

Yeah, I was thinking something along the same lines, but upon consulting mjordan and dvossel, I was feeling rather discouraged from doing this.  According to the man pages, even if fclose fails there is no guarantee that the file descriptor is still valid.  Here's an excerpt from fclose.

DESCRIPTION
       The  fclose()  function flushes the stream pointed to by fp (writing any buffered output data
       using fflush(3)) and closes the underlying file descriptor.

       The behaviour of fclose() is undefined if the stream parameter is an illegal pointer, or is a
       descriptor already passed to a previous invocation of fclose().

RETURN VALUE
       Upon successful completion 0 is returned.  Otherwise, EOF is returned and the global variable
       errno is set to indicate the error.  In either case any  further  access  (including  another
       call to fclose()) to the stream results in undefined behavior.

It seems pretty foreboding to me.


- jrose


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


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/20111111/e07eee6e/attachment-0001.htm>