[Asterisk-code-review] res http websocket: Forcefully terminate on write errors. (asterisk[11])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_http_websocket: Forcefully terminate on write errors.
......................................................................


res_http_websocket: Forcefully terminate on write errors.

The res_http_websocket module will currently attempt to close
the WebSocket connection if fatal cases occur, such as when
attempting to write out data and being unable to. When the
fatal cases occur the code attempts to write a WebSocket close
frame out to have the remote side close the connection. If
writing this fails then the connection is not terminated.

This change forcefully terminates the connection if the
WebSocket is to be closed but is unable to send the close frame.

ASTERISK-25312 #close

Change-Id: I10973086671cc192a76424060d9ec8e688602845
---
M res/res_http_websocket.c
1 file changed, 29 insertions(+), 5 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Mark Michelson: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/res/res_http_websocket.c b/res/res_http_websocket.c
index f12c729..47c1557 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -209,6 +209,17 @@
 
 	ao2_lock(session);
 	res = ast_careful_fwrite(session->f, session->fd, frame, 4, session->timeout);
+
+	/* If an error occurred when trying to close this connection explicitly terminate it now.
+	 * Doing so will cause the thread polling on it to wake up and terminate.
+	 */
+	if (res) {
+		fclose(session->f);
+		session->f = NULL;
+		ast_verb(2, "WebSocket connection from '%s' forcefully closed due to fatal write error\n",
+			ast_sockaddr_stringify(&session->address));
+	}
+
 	ao2_unlock(session);
 
 	return res;
@@ -366,6 +377,13 @@
 	char *rbuf = buf;
 	int sanity = 10;
 
+	ao2_lock(session);
+	if (!session->f) {
+		ao2_unlock(session);
+		errno = ECONNABORTED;
+		return -1;
+	}
+
 	for (;;) {
 		clearerr(session->f);
 		rlen = fread(rbuf, 1, xlen, session->f);
@@ -374,6 +392,7 @@
 				ast_log(LOG_WARNING, "Web socket closed abruptly\n");
 				*opcode = AST_WEBSOCKET_OPCODE_CLOSE;
 				session->closing = 1;
+				ao2_unlock(session);
 				return -1;
 			}
 
@@ -381,6 +400,7 @@
 				ast_log(LOG_ERROR, "Error reading from web socket: %s\n", strerror(errno));
 				*opcode = AST_WEBSOCKET_OPCODE_CLOSE;
 				session->closing = 1;
+				ao2_unlock(session);
 				return -1;
 			}
 
@@ -388,6 +408,7 @@
 				ast_log(LOG_WARNING, "Websocket seems unresponsive, disconnecting ...\n");
 				*opcode = AST_WEBSOCKET_OPCODE_CLOSE;
 				session->closing = 1;
+				ao2_unlock(session);
 				return -1;
 			}
 		}
@@ -400,9 +421,12 @@
 			ast_log(LOG_ERROR, "ast_wait_for_input returned err: %s\n", strerror(errno));
 			*opcode = AST_WEBSOCKET_OPCODE_CLOSE;
 			session->closing = 1;
+			ao2_unlock(session);
 			return -1;
 		}
 	}
+
+	ao2_unlock(session);
 	return 0;
 }
 
@@ -419,7 +443,7 @@
 	*fragmented = 0;
 
 	if (ws_safe_read(session, &buf[0], MIN_WS_HDR_SZ, opcode)) {
-		return 0;
+		return -1;
 	}
 	frame_size += MIN_WS_HDR_SZ;
 
@@ -437,7 +461,7 @@
 		if (options_len) {
 			/* read the rest of the header options */
 			if (ws_safe_read(session, &buf[frame_size], options_len, opcode)) {
-				return 0;
+				return -1;
 			}
 			frame_size += options_len;
 		}
@@ -466,7 +490,7 @@
 		}
 
 		if (ws_safe_read(session, *payload, *payload_len, opcode)) {
-			return 0;
+			return -1;
 		}
 
 		/* If a mask is present unmask the payload */
@@ -490,7 +514,7 @@
 					session->payload, session->payload_len, *payload_len);
 				*payload_len = 0;
 				ast_websocket_close(session, 1009);
-				return 0;
+				return -1;
 			}
 
 			session->payload = new_payload;
@@ -527,7 +551,7 @@
 		/* Make the payload available so the user can look at the reason code if they so desire */
 		if ((*payload_len) && (new_payload = ast_realloc(session->payload, *payload_len))) {
 			if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
-				return 0;
+				return -1;
 			}
 			session->payload = new_payload;
 			memcpy(session->payload, &buf[frame_size], *payload_len);

-- 
To view, visit https://gerrit.asterisk.org/1072
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I10973086671cc192a76424060d9ec8e688602845
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>