[Asterisk-code-review] res http websocket: Avoid passing strlen() to ast websocket ... (asterisk[13])

[Mark Michelson]

Mark Michelson has uploaded a new change for review.

  https://gerrit.asterisk.org/1031

Change subject: res_http_websocket: Avoid passing strlen() to ast_websocket_write().
......................................................................

res_http_websocket: Avoid passing strlen() to ast_websocket_write().

We have seen a rash of test failures on a 32-bit build agent. Commit
48698a5e21d7307f61b5fb2bd39fd593bc1423ca solved an obvious problem where
we were not encoding a 64-bit value correctly over the wire. This
commit, however, did not solve the test failures.

In the failing tests, ARI is attempting to send a 537 byte text frame
over a websocket. When sending a frame this small, 16 bits are all that
is required in order to encode the payload length on the websocket
frame. However, ast_websocket_write() thinks that the payload length is
greater than 65535 and therefore writes out a 64 bit payload length.
Inspecting this payload length, the lower 32 bits are exactly what we
would expect it to be, 537 in hex. The upper 32 bits, are junk values
that are not expected to be there.

In the failure, we are passing the result of strlen() to a function that
expects a uint64_t parameter to be passed in. strlen() returns a size_t,
which on this 32-bit machine is 32 bits wide. Normally, passing a 32-bit
unsigned value to somewhere where a 64-bit unsigned value is expected
would cause no problems. In fact, in manual runs of failing tests, this
works just fine. However, ast_websocket_write() uses the Asterisk
optional API, which means that rather than a simple function call, there
are a series of macros that are used for its declaration and
implementation. These macros may be causing some sort of error to occur
when converting from a 32 bit quantity to a 64 bit quantity.

This commit changes the logic by making existing ast_websocket_write()
calls use ast_websocket_write_string() instead. Within
ast_websocket_write_string(), the 64-bit converted strlen is saved in a
local variable, and that variable is passed to ast_websocket_write()
instead.

Note that this commit message is full of speculation rather than
certainty. This is because the observed test failures, while always
present in automated test runs, never occur when tests are manually
attempted on the same test agent. The idea behind this commit is to fix
a theoretical issue by performing changes that should, at the least,
cause no harm. If it turns out that this change does not fix the failing
tests, then this commit should be reverted.

Change-Id: I4458dd87d785ca322b89c152b223a540a3d23e67
---
M channels/chan_sip.c
M res/ari/ari_websockets.c
M res/res_http_websocket.c
M res/res_pjsip_transport_websocket.c
4 files changed, 15 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/31/1031/1

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 9ab429e..9ba0e19 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -3717,7 +3717,7 @@
 	} else if (p->socket.tcptls_session) {
 		res = sip_tcptls_write(p->socket.tcptls_session, ast_str_buffer(data), ast_str_strlen(data));
 	} else if (p->socket.ws_session) {
-		if (!(res = ast_websocket_write(p->socket.ws_session, AST_WEBSOCKET_OPCODE_TEXT, ast_str_buffer(data), ast_str_strlen(data)))) {
+		if (!(res = ast_websocket_write_string(p->socket.ws_session, ast_str_buffer(data)))) {
 			/* The WebSocket API just returns 0 on success and -1 on failure, while this code expects the payload length to be returned */
 			res = ast_str_strlen(data);
 		}
diff --git a/res/ari/ari_websockets.c b/res/ari/ari_websockets.c
index f3b764b..1d2eacd 100644
--- a/res/ari/ari_websockets.c
+++ b/res/ari/ari_websockets.c
@@ -163,9 +163,7 @@
 #ifdef AST_DEVMODE
 	if (!session->validator(message)) {
 		ast_log(LOG_ERROR, "Outgoing message failed validation\n");
-		return ast_websocket_write(session->ws_session,
-			AST_WEBSOCKET_OPCODE_TEXT, VALIDATION_FAILED,
-			strlen(VALIDATION_FAILED));
+		return ast_websocket_write_string(session->ws_session, VALIDATION_FAILED);
 	}
 #endif
 
@@ -177,8 +175,7 @@
 	}
 
 	ast_debug(3, "Examining ARI event: \n%s\n", str);
-	if (ast_websocket_write(session->ws_session,
-				AST_WEBSOCKET_OPCODE_TEXT, str,	strlen(str))) {
+	if (ast_websocket_write_string(session->ws_session, str)) {
 		ast_log(LOG_NOTICE, "Problem occurred during websocket write, websocket closed\n");
 		return -1;
 	}
diff --git a/res/res_http_websocket.c b/res/res_http_websocket.c
index 144c08d..bad8337 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -1354,8 +1354,17 @@
 int AST_OPTIONAL_API_NAME(ast_websocket_write_string)
 	(struct ast_websocket *ws, const char *buf)
 {
+	uint64_t len = strlen(buf);
+
+	/* We do not pass strlen(buf) to ast_websocket_write() directly because the
+	 * size_t returned by strlen() may not require the same storage size
+	 * as the uint64_t that ast_websocket_write() uses. This normally
+	 * would not cause a problem, but since ast_websocket_write() uses
+	 * the optional API, this function call goes through a series of macros
+	 * that may cause a 32-bit to 64-bit conversion to go awry.
+	 */
 	return ast_websocket_write(ws, AST_WEBSOCKET_OPCODE_TEXT,
-				   (char *)buf, strlen(buf));
+				   (char *)buf, len);
 }
 
 static int load_module(void)
diff --git a/res/res_pjsip_transport_websocket.c b/res/res_pjsip_transport_websocket.c
index be30468..f2c16a1 100644
--- a/res/res_pjsip_transport_websocket.c
+++ b/res/res_pjsip_transport_websocket.c
@@ -63,8 +63,9 @@
                             pjsip_transport_callback callback)
 {
 	struct ws_transport *wstransport = (struct ws_transport *)transport;
+	uint64_t len = tdata->buf.cur - tdata->buf.start;
 
-	if (ast_websocket_write(wstransport->ws_session, AST_WEBSOCKET_OPCODE_TEXT, tdata->buf.start, (int)(tdata->buf.cur - tdata->buf.start))) {
+	if (ast_websocket_write(wstransport->ws_session, AST_WEBSOCKET_OPCODE_TEXT, tdata->buf.start, len)) {
 		return PJ_EUNKNOWN;
 	}
 

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4458dd87d785ca322b89c152b223a540a3d23e67
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>