res_http_websocket: Close websocket correctly and use careful fwrite

When a client takes a long time to process information received from Asterisk,
a write operation using fwrite may fail to write all information. This causes
the underlying file stream to be in an unknown state, such that the socket
must be disconnected. Unfortunately, there are two problems with this in
Asterisk's existing websocket code:
1. Periodically, during the read loop, Asterisk must write to the connected
   websocket to respond to pings. As such, Asterisk maintains a reference to
   the session during the loop. When ast_http_websocket_write fails, it may
   cause the session to decrement its ref count, but this in and of itself
   does not break the read loop. The read loop's write, on the other hand,
   does not break the loop if it fails. This causes the socket to get in a
   'stuck' state, preventing the client from reconnecting to the server.
2. More importantly, however, is that the fwrite in ast_http_websocket_write
   fails with a large volume of data when the client takes awhile to process
   the information. When it does fail, it fails writing only a portion of
   the bytes. With some debugging, it was shown that this was failing in a
   similar fashion to ASTERISK-12767. Switching this over to ast_careful_fwrite
   with a long enough timeout solved the problem.

Note that this version of the patch, unlike r417310 in Asterisk 11, exposes
configuration options beyond just chan_sip's sip.conf. Configuration options
to configure the write timeout have also been added to pjsip.conf and ari.conf.

#ASTERISK-23917 #close
Reported by: Matt Jordan

Review: https://reviewboard.asterisk.org/r/3624/
........

Merged revisions 417310 from http://svn.asterisk.org/svn/asterisk/branches/11
........

Merged revisions 417311 from http://svn.asterisk.org/svn/asterisk/branches/12


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@417317 65c4cc65-6c06-0410-ace0-fbb531ad65f3

8 files changed, 86 insertions, 6 deletions

diff --git a/res/ari/ari_websockets.c b/res/ari/ari_websockets.c
index 90d6f0fdb..ff0a53c4f 100644
--- a/res/ari/ari_websockets.c
+++ b/res/ari/ari_websockets.c
@@ -56,11 +56,16 @@ struct ast_ari_websocket_session *ast_ari_websocket_session_create(
 	struct ast_websocket *ws_session, int (*validator)(struct ast_json *))
 {
 	RAII_VAR(struct ast_ari_websocket_session *, session, NULL, ao2_cleanup);
+	RAII_VAR(struct ast_ari_conf *, config, ast_ari_config_get(), ao2_cleanup);
 
 	if (ws_session == NULL) {
 		return NULL;
 	}
 
+	if (config == NULL || config->general == NULL) {
+		return NULL;
+	}
+
 	if (validator == NULL) {
 		validator = null_validator;
 	}
@@ -72,6 +77,11 @@ struct ast_ari_websocket_session *ast_ari_websocket_session_create(
 		return NULL;
 	}
 
+	if (ast_websocket_set_timeout(ws_session, config->general->write_timeout)) {
+		ast_log(LOG_WARNING, "Failed to set write timeout %d on ARI web socket\n",
+			config->general->write_timeout);
+	}
+
 	session = ao2_alloc(sizeof(*session), websocket_session_dtor);
 	if (!session) {
 		return NULL;
diff --git a/res/ari/config.c b/res/ari/config.c
index 59c4d7d94..667d91ac0 100644
--- a/res/ari/config.c
+++ b/res/ari/config.c
@@ -27,6 +27,7 @@
 ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 
 #include "asterisk/config_options.h"
+#include "asterisk/http_websocket.h"
 #include "internal.h"
 
 /*! \brief Locking container for safe configuration access. */
@@ -320,6 +321,9 @@ int ast_ari_config_init(void)
 	aco_option_register(&cfg_info, "allowed_origins", ACO_EXACT, general_options,
 		"", OPT_STRINGFIELD_T, 0,
 		STRFLDSET(struct ast_ari_conf_general, allowed_origins));
+	aco_option_register(&cfg_info, "websocket_write_timeout", ACO_EXACT, general_options,
+		AST_DEFAULT_WEBSOCKET_WRITE_TIMEOUT_STR, OPT_INT_T, PARSE_IN_RANGE,
+		FLDSET(struct ast_ari_conf_general, write_timeout), 1, INT_MAX);
 
 	aco_option_register(&cfg_info, "type", ACO_EXACT, user, NULL,
 		OPT_NOOP_T, 0, 0);
diff --git a/res/ari/internal.h b/res/ari/internal.h
index 8453747f1..93ea0b773 100644
--- a/res/ari/internal.h
+++ b/res/ari/internal.h
@@ -65,6 +65,8 @@ struct ast_ari_conf {
 struct ast_ari_conf_general {
 	/*! Enabled by default, disabled if false. */
 	int enabled;
+	/*! Write timeout for websocket connections */
+	int write_timeout;
 	/*! Encoding format used during output (default compact). */
 	enum ast_json_encoding_format format;
 	/*! Authentication realm */
diff --git a/res/res_ari.c b/res/res_ari.c
index ce7027e44..acdbbfe9a 100644
--- a/res/res_ari.c
+++ b/res/res_ari.c
@@ -95,6 +95,14 @@
 						<ref type="link">https://wiki.asterisk.org/wiki/display/AST/Asterisk+Builtin+mini-HTTP+Server</ref>
 					</see-also>
 				</configOption>
+				<configOption name="websocket_write_timeout">
+					<synopsis>The timeout (in milliseconds) to set on WebSocket connections.</synopsis>
+					<description>
+						<para>If a websocket connection accepts input slowly, the timeout
+						for writes to it can be increased to keep it from being disconnected.
+						Value is in milliseconds; default is 100 ms.</para>
+					</description>
+				</configOption>
 				<configOption name="pretty">
 					<synopsis>Responses from ARI are formatted to be human readable</synopsis>
 				</configOption>
diff --git a/res/res_http_websocket.c b/res/res_http_websocket.c
index 07fcd9e2e..90744a1b3 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -81,6 +81,7 @@ struct ast_websocket {
 	size_t payload_len;               /*!< Length of the payload */
 	char *payload;                    /*!< Pointer to the payload */
 	size_t reconstruct;               /*!< Number of bytes before a reconstructed payload will be returned and a new one started */
+	int timeout;                      /*!< The timeout for operations on the socket */
 	unsigned int secure:1;            /*!< Bit to indicate that the transport is secure */
 	unsigned int closing:1;           /*!< Bit to indicate that the session is in the process of being closed */
 	unsigned int close_sent:1;        /*!< Bit to indicate that the session close opcode has been sent and no further data will be sent */
@@ -260,7 +261,7 @@ int AST_OPTIONAL_API_NAME(ast_websocket_close)(struct ast_websocket *session, ui
 	session->close_sent = 1;
 
 	ao2_lock(session);
-	res = (fwrite(frame, 1, 4, session->f) == 4) ? 0 : -1;
+	res = ast_careful_fwrite(session->f, session->fd, frame, 4, session->timeout);
 	ao2_unlock(session);
 	return res;
 }
@@ -303,13 +304,12 @@ int AST_OPTIONAL_API_NAME(ast_websocket_write)(struct ast_websocket *session, en
 		ao2_unlock(session);
 		return -1;
 	}
-
-	if (fwrite(frame, 1, header_size, session->f) != header_size) {
+	if (ast_careful_fwrite(session->f, session->fd, frame, header_size, session->timeout)) {
 		ao2_unlock(session);
 		return -1;
 	}
 
-	if (fwrite(payload, 1, actual_length, session->f) != actual_length) {
+	if (ast_careful_fwrite(session->f, session->fd, payload, actual_length, session->timeout)) {
 		ao2_unlock(session);
 		return -1;
 	}
@@ -371,6 +371,13 @@ int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *sess
 	return 0;
 }
 
+int AST_OPTIONAL_API_NAME(ast_websocket_set_timeout)(struct ast_websocket *session, int timeout)
+{
+	session->timeout = timeout;
+
+	return 0;
+}
+
 /* MAINTENANCE WARNING on ast_websocket_read()!
  *
  * We have to keep in mind during this function that the fact that session->fd seems ready
@@ -514,8 +521,10 @@ int AST_OPTIONAL_API_NAME(ast_websocket_read)(struct ast_websocket *session, cha
 		}
 
 		/* Per the RFC for PING we need to send back an opcode with the application data as received */
-		if (*opcode == AST_WEBSOCKET_OPCODE_PING) {
-			ast_websocket_write(session, AST_WEBSOCKET_OPCODE_PONG, *payload, *payload_len);
+		if ((*opcode == AST_WEBSOCKET_OPCODE_PING) && (ast_websocket_write(session, AST_WEBSOCKET_OPCODE_PONG, *payload, *payload_len))) {
+			*payload_len = 0;
+			ast_websocket_close(session, 1009);
+			return 0;
 		}
 
 		session->payload = new_payload;
@@ -696,6 +705,7 @@ int AST_OPTIONAL_API_NAME(ast_websocket_uri_cb)(struct ast_tcptls_session_instan
 			ao2_ref(protocol_handler, -1);
 			return 0;
 		}
+		session->timeout =  AST_DEFAULT_WEBSOCKET_WRITE_TIMEOUT;
 
 		fprintf(ser->f, "HTTP/1.1 101 Switching Protocols\r\n"
 			"Upgrade: %s\r\n"
diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 2602660ee..45b8e7e02 100644
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -869,6 +869,14 @@
 					or the <replaceable>wss</replaceable> protocols.</para></note>
 					</description>
 				</configOption>
+				<configOption name="websocket_write_timeout">
+					<synopsis>The timeout (in milliseconds) to set on WebSocket connections.</synopsis>
+					<description>
+						<para>If a websocket connection accepts input slowly, the timeout
+						for writes to it can be increased to keep it from being disconnected.
+						Value is in milliseconds; default is 100 ms.</para>
+					</description>
+				</configOption>
 			</configObject>
 			<configObject name="contact">
 				<synopsis>A way of creating an aliased name to a SIP URI</synopsis>
diff --git a/res/res_pjsip/config_transport.c b/res/res_pjsip/config_transport.c
index 22581ca52..785fcc5ac 100644
--- a/res/res_pjsip/config_transport.c
+++ b/res/res_pjsip/config_transport.c
@@ -28,6 +28,7 @@
 #include "asterisk/sorcery.h"
 #include "asterisk/acl.h"
 #include "include/res_pjsip_private.h"
+#include "asterisk/http_websocket.h"
 
 static int sip_transport_to_ami(const struct ast_sip_transport *transport,
 				struct ast_str **buf)
@@ -668,6 +669,7 @@ int ast_sip_initialize_sorcery_transport(void)
 	ast_sorcery_object_field_register_custom(sorcery, "transport", "local_net", "", transport_localnet_handler, localnet_to_str, localnet_to_vl, 0, 0);
 	ast_sorcery_object_field_register_custom(sorcery, "transport", "tos", "0", transport_tos_handler, tos_to_str, NULL, 0, 0);
 	ast_sorcery_object_field_register(sorcery, "transport", "cos", "0", OPT_UINT_T, 0, FLDSET(struct ast_sip_transport, cos));
+	ast_sorcery_object_field_register(sorcery, "transport", "websocket_write_timeout", AST_DEFAULT_WEBSOCKET_WRITE_TIMEOUT_STR, OPT_INT_T, PARSE_IN_RANGE, FLDSET(struct ast_sip_transport, write_timeout), 1, INT_MAX);
 
 	ast_sip_register_endpoint_formatter(&endpoint_transport_formatter);
 
diff --git a/res/res_pjsip_transport_websocket.c b/res/res_pjsip_transport_websocket.c
index 22962dab0..bae120a19 100644
--- a/res/res_pjsip_transport_websocket.c
+++ b/res/res_pjsip_transport_websocket.c
@@ -207,6 +207,37 @@ static int transport_read(void *data)
 	return (read_data->payload_len == recvd) ? 0 : -1;
 }
 
+static int get_write_timeout(void)
+{
+	int write_timeout = -1;
+	struct ao2_container *transports;
+
+	transports = ast_sorcery_retrieve_by_fields(ast_sip_get_sorcery(), "transport", AST_RETRIEVE_FLAG_ALL, NULL);
+
+	if (transports) {
+		struct ao2_iterator it_transports = ao2_iterator_init(transports, 0);
+		struct ast_sip_transport *transport;
+
+		for (; (transport = ao2_iterator_next(&it_transports)); ao2_cleanup(transport)) {
+			if (transport->type != AST_TRANSPORT_WS && transport->type != AST_TRANSPORT_WSS) {
+				continue;
+			}
+			ast_debug(5, "Found %s transport with write timeout: %d\n",
+				transport->type == AST_TRANSPORT_WS ? "WS" : "WSS",
+				transport->write_timeout);
+			write_timeout = MAX(write_timeout, transport->write_timeout);
+		}
+		ao2_cleanup(transports);
+	}
+
+	if (write_timeout < 0) {
+		write_timeout = AST_DEFAULT_WEBSOCKET_WRITE_TIMEOUT;
+	}
+
+	ast_debug(1, "Write timeout for WS/WSS transports: %d\n", write_timeout);
+	return write_timeout;
+}
+
 /*!
  \brief WebSocket connection handler.
  */
@@ -222,6 +253,11 @@ static void websocket_cb(struct ast_websocket *session, struct ast_variable *par
 		return;
 	}
 
+	if (ast_websocket_set_timeout(session, get_write_timeout())) {
+		ast_websocket_unref(session);
+		return;
+	}
+
 	if (!(serializer = ast_sip_create_serializer())) {
 		ast_websocket_unref(session);
 		return;