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

4 files changed, 15 insertions, 8 deletions

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 49a707edc..419077003 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -3721,7 +3721,7 @@ static int __sip_xmit(struct sip_pvt *p, struct ast_str *data)
 	} 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 9a4f2c9fe..ced1e0d69 100644
--- a/res/ari/ari_websockets.c
+++ b/res/ari/ari_websockets.c
@@ -163,9 +163,7 @@ int ast_ari_websocket_session_write(struct ast_ari_websocket_session *session,
 #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 @@ int ast_ari_websocket_session_write(struct ast_ari_websocket_session *session,
 	}
 
 	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 1ecfd964c..33905452f 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -1370,8 +1370,17 @@ int AST_OPTIONAL_API_NAME(ast_websocket_read_string)
 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 e98756fdf..e48b630e7 100644
--- a/res/res_pjsip_transport_websocket.c
+++ b/res/res_pjsip_transport_websocket.c
@@ -63,8 +63,9 @@ static pj_status_t ws_send_msg(pjsip_transport *transport,
                             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;
 	}