[Asterisk-code-review] core/tcptls: fix double socket close() on failed connection to offlin... (...asterisk[16])

[under]

under has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/11419


Change subject: core/tcptls: fix double socket close() on failed connection to offline TCP peer
......................................................................

core/tcptls: fix double socket close() on failed connection to offline TCP peer

* Eliminate double socket close() on failed connection to offline TCP peer
  by removing close() from ast_tcptls_client_start().
  1st time socket is closed inside ast_tcptls_client_start().
  2nd time socket is closed inside ast_iostream_close(),
  when invoked from session_instance_destructor().
  Double socket close() is extremely bad,
  because after 1st close() the socket FD could be re-used
  by some other Asterisk thread.
  And 2nd close() will close the FD in a totally different thread,
  causing issues in it: asserts, call hangups, and other unpredictable things.

* Eliminate using desc->accept_fd in ast_tcptls_client_start()
  and ast_tcptls_client_create().
  TCP accept() is relevant only to TCP server code,
  and is not relevant to TCP client code.
  Therefore usage of accept_fd in TCP client code
  might confuse whoever reads the code.

* Eliminate saving socket FD in desc->accept_fd
  after ast_iostream_from_fd() has been invoked.
  ast_iostream_from_fd() has "socket FD ownership transfer" semantics.
  Therefore, saving FD elsewhere besides iostream breaks
  FD incapsulation inside iostream,
  and makes possible future double close() issues.

ASTERISK-28430 #close

Change-Id: Idf0f7f5b4b304c37e89ef8352cbf976bebf96342
---
M main/tcptls.c
1 file changed, 14 insertions(+), 13 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/19/11419/1

diff --git a/main/tcptls.c b/main/tcptls.c
index 7930c50..eb02f75 100644
--- a/main/tcptls.c
+++ b/main/tcptls.c
@@ -549,7 +549,13 @@
 		goto client_start_error;
 	}
 
-	if (ast_connect(desc->accept_fd, &desc->remote_address)) {
+	if (!tcptls_session->stream) {
+		/* We shouldn't be here if stream is not allocated */
+		ast_assert(0);
+		goto client_start_error;
+	}
+
+	if (ast_connect(ast_iostream_get_fd(tcptls_session->stream), &desc->remote_address)) {
 		ast_log(LOG_ERROR, "Unable to connect %s to %s: %s\n",
 			desc->name,
 			ast_sockaddr_stringify(&desc->remote_address),
@@ -557,7 +563,7 @@
 		goto client_start_error;
 	}
 
-	ast_fd_clear_flags(desc->accept_fd, O_NONBLOCK);
+	ast_fd_clear_flags(ast_iostream_get_fd(tcptls_session->stream), O_NONBLOCK);
 
 	if (desc->tls_cfg) {
 		desc->tls_cfg->enabled = 1;
@@ -567,10 +573,6 @@
 	return handle_tcptls_connection(tcptls_session);
 
 client_start_error:
-	if (desc) {
-		close(desc->accept_fd);
-		desc->accept_fd = -1;
-	}
 	ao2_ref(tcptls_session, -1);
 	return NULL;
 
@@ -594,9 +596,9 @@
 		close(desc->accept_fd);
 	}
 
-	fd = desc->accept_fd = socket(ast_sockaddr_is_ipv6(&desc->remote_address) ?
-				      AF_INET6 : AF_INET, SOCK_STREAM, IPPROTO_TCP);
-	if (desc->accept_fd < 0) {
+	fd = socket(ast_sockaddr_is_ipv6(&desc->remote_address) ?
+		    AF_INET6 : AF_INET, SOCK_STREAM, IPPROTO_TCP);
+	if (fd < 0) {
 		ast_log(LOG_ERROR, "Unable to allocate socket for %s: %s\n",
 			desc->name, strerror(errno));
 		return NULL;
@@ -606,8 +608,8 @@
 	   originate from the desired address */
 	if (!ast_sockaddr_isnull(&desc->local_address) &&
 	    !ast_sockaddr_is_any(&desc->local_address)) {
-		setsockopt(desc->accept_fd, SOL_SOCKET, SO_REUSEADDR, &x, sizeof(x));
-		if (ast_bind(desc->accept_fd, &desc->local_address)) {
+		setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &x, sizeof(x));
+		if (ast_bind(fd, &desc->local_address)) {
 			ast_log(LOG_ERROR, "Unable to bind %s to %s: %s\n",
 				desc->name,
 				ast_sockaddr_stringify(&desc->local_address),
@@ -641,8 +643,7 @@
 	return tcptls_session;
 
 error:
-	close(desc->accept_fd);
-	desc->accept_fd = -1;
+	close(fd);
 	ao2_cleanup(tcptls_session);
 	return NULL;
 }

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11419
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Idf0f7f5b4b304c37e89ef8352cbf976bebf96342
Gerrit-Change-Number: 11419
Gerrit-PatchSet: 1
Gerrit-Owner: under <under at list.ru>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190529/6272b40c/attachment-0001.html>