[Asterisk-code-review] websocket / pjsip: Increase maximum packet size. (asterisk[certified/16.8])

Joshua Colp asteriskteam at digium.com
Thu Jul 23 13:50:39 CDT 2020


Joshua Colp has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/14712 )

Change subject: websocket / pjsip: Increase maximum packet size.
......................................................................

websocket / pjsip: Increase maximum packet size.

When dealing with a lot of video streams on WebRTC
the resulting SDPs can grow to be quite large. This
effectively doubles the maximum size to allow more
streams to exist.

The res_http_websocket module has also been changed
to use a buffer on the session for reading in packets
to ensure that the stack space usage is not excessive.

Change-Id: I31d4351d70c8e2c11564807a7528b984f3fbdd01
---
M res/res_http_websocket.c
M res/res_pjsip_pubsub.c
M third-party/pjproject/patches/config_site.h
3 files changed, 21 insertions(+), 29 deletions(-)

Approvals:
  George Joseph: Looks good to me, but someone else must approve
  Kevin Harwell: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Approved for Submit



diff --git a/res/res_http_websocket.c b/res/res_http_websocket.c
index 63fccdd..0e7b89f 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -63,15 +63,15 @@
 #define MAXIMUM_RECONSTRUCTION_CEILING 8192
 #else
 /*! \brief Size of the pre-determined buffer for WebSocket frames */
-#define MAXIMUM_FRAME_SIZE 32768
+#define MAXIMUM_FRAME_SIZE 65535
 
 /*! \brief Default reconstruction size for multi-frame payload reconstruction. If exceeded the next frame will start a
  *         payload.
  */
-#define DEFAULT_RECONSTRUCTION_CEILING 32768
+#define DEFAULT_RECONSTRUCTION_CEILING MAXIMUM_FRAME_SIZE
 
 /*! \brief Maximum reconstruction size for multi-frame payload reconstruction. */
-#define MAXIMUM_RECONSTRUCTION_CEILING 32768
+#define MAXIMUM_RECONSTRUCTION_CEILING MAXIMUM_FRAME_SIZE
 #endif
 
 /*! \brief Maximum size of a websocket frame header
@@ -100,6 +100,7 @@
 	struct websocket_client *client;    /*!< Client object when connected as a client websocket */
 	char session_id[AST_UUID_STR_LEN];  /*!< The identifier for the websocket session */
 	uint16_t close_status_code;         /*!< Status code sent in a CLOSE frame upon shutdown */
+	char buf[MAXIMUM_FRAME_SIZE];	    /*!< Fixed buffer for reading data into */
 };
 
 /*! \brief Hashing function for protocols */
@@ -565,7 +566,6 @@
 
 int AST_OPTIONAL_API_NAME(ast_websocket_read)(struct ast_websocket *session, char **payload, uint64_t *payload_len, enum ast_websocket_opcode *opcode, int *fragmented)
 {
-	char buf[MAXIMUM_FRAME_SIZE] = "";
 	int fin = 0;
 	int mask_present = 0;
 	char *mask = NULL, *new_payload = NULL;
@@ -575,25 +575,25 @@
 	*payload_len = 0;
 	*fragmented = 0;
 
-	if (ws_safe_read(session, &buf[0], MIN_WS_HDR_SZ, opcode)) {
+	if (ws_safe_read(session, &session->buf[0], MIN_WS_HDR_SZ, opcode)) {
 		return -1;
 	}
 	frame_size += MIN_WS_HDR_SZ;
 
 	/* ok, now we have the first 2 bytes, so we know some flags, opcode and payload length (or whether payload length extension will be required) */
-	*opcode = buf[0] & 0xf;
-	*payload_len = buf[1] & 0x7f;
+	*opcode = session->buf[0] & 0xf;
+	*payload_len = session->buf[1] & 0x7f;
 	if (*opcode == AST_WEBSOCKET_OPCODE_TEXT || *opcode == AST_WEBSOCKET_OPCODE_BINARY || *opcode == AST_WEBSOCKET_OPCODE_CONTINUATION ||
 	    *opcode == AST_WEBSOCKET_OPCODE_PING || *opcode == AST_WEBSOCKET_OPCODE_PONG  || *opcode == AST_WEBSOCKET_OPCODE_CLOSE) {
-		fin = (buf[0] >> 7) & 1;
-		mask_present = (buf[1] >> 7) & 1;
+		fin = (session->buf[0] >> 7) & 1;
+		mask_present = (session->buf[1] >> 7) & 1;
 
 		/* Based on the mask flag and payload length, determine how much more we need to read before start parsing the rest of the header */
 		options_len += mask_present ? 4 : 0;
 		options_len += (*payload_len == 126) ? 2 : (*payload_len == 127) ? 8 : 0;
 		if (options_len) {
 			/* read the rest of the header options */
-			if (ws_safe_read(session, &buf[frame_size], options_len, opcode)) {
+			if (ws_safe_read(session, &session->buf[frame_size], options_len, opcode)) {
 				return -1;
 			}
 			frame_size += options_len;
@@ -601,19 +601,19 @@
 
 		if (*payload_len == 126) {
 			/* Grab the 2-byte payload length  */
-			*payload_len = ntohs(get_unaligned_uint16(&buf[2]));
-			mask = &buf[4];
+			*payload_len = ntohs(get_unaligned_uint16(&session->buf[2]));
+			mask = &session->buf[4];
 		} else if (*payload_len == 127) {
 			/* Grab the 8-byte payload length  */
-			*payload_len = ntohl(get_unaligned_uint64(&buf[2]));
-			mask = &buf[10];
+			*payload_len = ntohll(get_unaligned_uint64(&session->buf[2]));
+			mask = &session->buf[10];
 		} else {
 			/* Just set the mask after the small 2-byte header */
-			mask = &buf[2];
+			mask = &session->buf[2];
 		}
 
 		/* Now read the rest of the payload */
-		*payload = &buf[frame_size]; /* payload will start here, at the end of the options, if any */
+		*payload = &session->buf[frame_size]; /* payload will start here, at the end of the options, if any */
 		frame_size = frame_size + (*payload_len); /* final frame size is header + optional headers + payload data */
 		if (frame_size > MAXIMUM_FRAME_SIZE) {
 			ast_log(LOG_WARNING, "Cannot fit huge websocket frame of %zu bytes\n", frame_size);
diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index bfa3dc9..2dbe874 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -146,8 +146,8 @@
 					that a server has to process.</para>
 					<note>
 						<para>Current limitations limit the size of SIP NOTIFY requests that Asterisk sends
-						to 64000 bytes. If your resource list notifications are larger than this maximum, you
-						will need to make adjustments.</para>
+						to double that of the PJSIP maximum packet length. If your resource list notifications
+						are larger than this maximum, you will need to make adjustments.</para>
 					</note>
 				</description>
 				<configOption name="type">
@@ -1948,15 +1948,7 @@
  * we instead take the strategy of pre-allocating the buffer, testing for ourselves
  * if the message will fit, and resizing the buffer as required.
  *
- * RFC 3261 says that a SIP UDP request can be up to 65535 bytes long. We're capping
- * it at 64000 for a couple of reasons:
- * 1) Allocating more than 64K at a time is hard to justify
- * 2) If the message goes through proxies, those proxies will want to add Via and
- *    Record-Route headers, making the message even larger. Giving some space for
- *    those headers is a nice thing to do.
- *
- * RFC 3261 does not place an upper limit on the size of TCP requests, but we are
- * going to impose the same 64K limit as a memory savings.
+ * The limit we impose is double that of the maximum packet length.
  *
  * \param tdata The tdata onto which to allocate a buffer
  * \retval 0 Success
@@ -1968,7 +1960,7 @@
 	int size = -1;
 	char *buf;
 
-	for (buf_size = PJSIP_MAX_PKT_LEN; size == -1 && buf_size < 64000; buf_size *= 2) {
+	for (buf_size = PJSIP_MAX_PKT_LEN; size == -1 && buf_size < (PJSIP_MAX_PKT_LEN * 2); buf_size *= 2) {
 		buf = pj_pool_alloc(tdata->pool, buf_size);
 		size = pjsip_msg_print(tdata->msg, buf, buf_size);
 	}
diff --git a/third-party/pjproject/patches/config_site.h b/third-party/pjproject/patches/config_site.h
index aea7d0d..5884108 100644
--- a/third-party/pjproject/patches/config_site.h
+++ b/third-party/pjproject/patches/config_site.h
@@ -65,7 +65,7 @@
   Enabling it will result in SEGFAULTS when URIs containing escape sequences are encountered.
 */
 #undef PJSIP_UNESCAPE_IN_PLACE
-#define PJSIP_MAX_PKT_LEN			32000
+#define PJSIP_MAX_PKT_LEN			65535
 
 #undef PJ_TODO
 #define PJ_TODO(x)

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

Gerrit-Project: asterisk
Gerrit-Branch: certified/16.8
Gerrit-Change-Id: I31d4351d70c8e2c11564807a7528b984f3fbdd01
Gerrit-Change-Number: 14712
Gerrit-PatchSet: 3
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200723/44f82522/attachment-0001.html>


More information about the asterisk-code-review mailing list