Summary:	ASTERISK-18700: chan_sip.c and tcptls.c - possible double close of file descriptor
Reporter:	Erik Wallin (erikw@pushtalk.se)	Labels:
Date Opened:	2011-10-10 10:16:00	Date Closed:	2011-11-30 15:16:15.000-0600
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Channels/chan_sip/TCP-TLS
Versions:	1.8.7.0	Frequency of Occurrence	Frequent
Related Issues:	is a clone of ASTERISK-18453 manager.c: HTTP Manager, fdopen failed: Bad file descriptor! is related to ASTERISK-18342 close() before SSL_Shutdown() in ssl_close() is related to ASTERISK-18345 [patch] sips connection dropped by asterisk with a large INVITE
Environment:		Attachments:
Description:	While researching another bug I found this potential issue. I think it would be worthwhile to investigate it, to make sure it is not an issue of the same kind as the first bug. The background is that in some places a file descriptor and FILE pointer is used for the same open file. When closing the file it's important that only one of these is closed. If not, a race condition would/could occur. A file descriptor opened in an unrelated thread would be closed by mistake. I looked through the entire asterisk source (as of 1.8.7.0) for such cases. I found two suspicious fclose/close pairs. channels/chan_sip.c line 2784 (1.8.7.0): if (tcptls_session->f) { fclose(tcptls_session->f); tcptls_session->f = NULL; } if (tcptls_session->fd != -1) { close(tcptls_session->fd); tcptls_session->fd = -1; } If I interpret include/asterisk/tcptls.h correctly f is a stream connected to fd. If SSL is enabled, there is also some encryption going on in the stream buffering code. I would therefore guess that the stream functions are overloaded with asterisk specific ones. The above code would lead to a race condition if the overloaded fclose has the same semantics as regular stream IO fclose. That is - if it closes the underlying fd when the stream is closed. If an unrelated open is issued in a different thread between the fclose - close statements, and that open returns the same file descriptor number, the wrong file would be closed. This might sound like an unlikely event. However the same file descriptor numbers are used over and over again in a process, as they are closed and opened. Given the rate at which they are created in a running asterisk process, this will (and have) eventually lead to the wrong file being closed. Another potential issue, involving the same data structure: main/tcptls.c line 206 (1.8.7.0): close(tcptls_session->fd); fclose(tcptls_session->f); The situation in main/tcptls.c is actually even worse, since the fd is closed before the user space buffer is flushed. It might be better to use fileno() than having separate member for the file descriptor and the stream pointer. The fd is already a member of the stream struct. Why duplicate it? Performance? fileno() locks the stream struct when it's accessed. But then there's fileno_unlocked(). Still that's a function call, so it is a bit of overhead.
Comments: