truncated: detect and flag any write on a wsi that could have pending truncated

READMEs/README.coding.md

@@ -44,7 +44,7 @@ clients).
 If you want to send something, do NOT just send it but request a callback
 when the socket is writeable using
 
- - `lws_callback_on_writable(context, wsi)` for a specific `wsi`, or
+ - `lws_callback_on_writable(wsi)` for a specific `wsi`, or
  
  - `lws_callback_on_writable_all_protocol(protocol)` for all connections
 using that protocol to get a callback when next writeable.
@@ -68,6 +68,25 @@ anything new for him.  This is how unix shell piping works, you may have
 `cat a.txt | grep xyz > remote", but actually that does not cat anything from
 a.txt while remote cannot accept anything new. 
 
+@section oneper Only one lws_write per WRITEABLE callback
+
+From v2.5, lws strictly enforces only one lws_write() per WRITEABLE callback.
+
+You will receive a message about "Illegal back-to-back write of ... detected"
+if there is a second lws_write() before returning to the event loop.
+
+This is because with http/2, the state of the network connection carrying a
+wsi is unrelated to any state of the wsi.  The situation on http/1 where a
+new request implied a new tcp connection and new SSL buffer, so you could
+assume some window for writes is no longer true.  Any lws_write() can fail
+and be buffered for completion by lws; it will be auto-completed by the
+event loop.
+
+Note that if you are handling your own http responses, writing the headers
+needs to be done with a separate lws_write() from writing any payload.  That
+means after writing the headers you must call `lws_callback_on_writable(wsi)`
+and send any payload from the writable callback.
+
 @section otherwr Do not rely on only your own WRITEABLE requests appearing
 
 Libwebsockets may generate additional `LWS_CALLBACK_CLIENT_WRITEABLE` events

lib/handshake.c

@@ -91,6 +91,28 @@ lws_read(struct lws *wsi, unsigned char *buf, lws_filepos_t len)
 				return 1;
 			}
 
+			/*
+			 * lws_h2_parser() may send something; when it gets the
+			 * whole frame, it will want to perform some action
+			 * involving a reply.  But we may be in a partial send
+			 * situation on the network wsi...
+			 *
+			 * Even though we may be in a partial send and unable to
+			 * send anything new, we still have to parse the network
+			 * wsi in order to gain tx credit to send, which is
+			 * potentially necessary to clear the old partial send.
+			 *
+			 * ALL network wsi-specific frames are sent by PPS
+			 * already, these are sent as a priority on the writable
+			 * handler, and so respect partial sends.  The only
+			 * problem is when a stream wsi wants to send an, eg,
+			 * reply headers frame in response to the parsing
+			 * we will do now... the *stream wsi* must stall in a
+			 * different state until it is able to do so from a
+			 * priority on the WRITABLE callback, same way that
+			 * file transfers operate.
+			 */
+
 			if (lws_h2_parser(wsi, buf, len, &body_chunk_len)) {
 				lwsl_debug("%s: http2_parser bailed\n", __func__);
 				goto bail;

lib/http2/hpack.c

@@ -598,13 +598,13 @@ lws_hpack_dynamic_size(struct lws *wsi, int size)
 
 	if (size > (int)nwsi->vhost->set.s[H2SET_HEADER_TABLE_SIZE]) {
 		lwsl_notice("rejecting hpack dyn size %u\n", size);
-#if defined(LWS_WITH_ESP32)
+//#if defined(LWS_WITH_ESP32)
 		size = nwsi->vhost->set.s[H2SET_HEADER_TABLE_SIZE];
-#else
-		lws_h2_goaway(nwsi, H2_ERR_COMPRESSION_ERROR,
-			"Asked for header table bigger than we told");
-		goto bail;
-#endif
+//#else
+//		lws_h2_goaway(nwsi, H2_ERR_COMPRESSION_ERROR,
+//			"Asked for header table bigger than we told");
+//		goto bail;
+//#endif
 	}
 
 	dyn->virtual_payload_max = size;

lib/http2/http2.c

@@ -1038,6 +1038,13 @@ update_end_headers:
 /*
  * The last byte of the whole frame has been handled.
  * Perform actions for frame completion.
+ *
+ * This is the crunch time for parsing that may have occured on a network
+ * wsi with a pending partial send... we may call lws_http_action() to send
+ * a response, conflicting with the partial.
+ *
+ * So in that case we change the wsi state and do the lws_http_action() in the
+ * WRITABLE handler as a priority.
  */
 static int
 lws_h2_parse_end_of_frame(struct lws *wsi)
@@ -1196,22 +1203,8 @@ lws_h2_parse_end_of_frame(struct lws *wsi)
 
 		wsi->vhost->conn_stats.h2_trans++;
 
-		lwsl_info("  action start...\n");
-		n = lws_http_action(h2n->swsi);
-		lwsl_info("  action result %d "
-			  "(wsi->http.rx_content_remain %lld)\n",
-			  n, h2n->swsi->http.rx_content_remain);
-
-		/*
-		 * Commonly we only managed to start a larger transfer that will
-		 * complete asynchronously.  In those cases we will hear about
-		 * END_STREAM going out in the POLLOUT handler.
-		 */
-		if (n || h2n->swsi->h2.send_END_STREAM) {
-			lws_close_free_wsi(h2n->swsi, 0);
-			h2n->swsi = NULL;
-			break;
-		}
+		h2n->swsi->state = LWSS_HTTP2_DEFERRING_ACTION;
+		lws_callback_on_writable(h2n->swsi);
 		break;
 
 	case LWS_H2_FRAME_TYPE_DATA:
@@ -1318,6 +1311,19 @@ lws_h2_parse_end_of_frame(struct lws *wsi)
 	return 0;
 }
 
+/*
+ * This may want to send something on the network wsi, which may be in the
+ * middle of a partial send.  PPS sends are OK because they are queued to
+ * go through the WRITABLE handler already.
+ *
+ * The read parser for the network wsi has no choice but to parse its stream
+ * anyway, because otherwise it will not be able to get tx credit window
+ * messages.
+ *
+ * Therefore if we will send non-PPS, ie, lws_http_action() for a stream
+ * wsi, we must change its state and handle it as a priority in the
+ * POLLOUT handler instead of writing it here.
+ */
 int
 lws_h2_parser(struct lws *wsi, unsigned char *in, lws_filepos_t inlen,
 	      lws_filepos_t *inused)

lib/libwebsockets.c

@@ -724,7 +724,7 @@ just_kill_connection:
 		    lws_sockfd_valid(wsi->desc.sockfd) &&
 		    !LWS_LIBUV_ENABLED(context)) {
 			lws_change_pollfd(wsi, LWS_POLLOUT, LWS_POLLIN);
-			wsi->state = LWSS_SHUTDOWN;
+			wsi->state = (wsi->state & ~0x1f) | LWSS_SHUTDOWN;
 			lws_set_timeout(wsi, PENDING_TIMEOUT_SHUTDOWN_FLUSH,
 					context->timeout_secs);
 

lib/output.c

@@ -51,6 +51,23 @@ int lws_issue_raw(struct lws *wsi, unsigned char *buf, size_t len)
 	unsigned int n;
 	int m;
 
+	/*
+	 * Detect if we got called twice without going through the
+	 * event loop to handle pending.  This would be caused by either
+	 * back-to-back writes in one WRITABLE (illegal) or calling lws_write()
+	 * from outside the WRITABLE callback (illegal).
+	 */
+	if (wsi->could_have_pending) {
+		lwsl_hexdump_level(LLL_ERR, buf, len);
+		lwsl_err("** %p: vh: %s, prot: %s, "
+			 "Illegal back-to-back write of %lu detected...\n",
+			 wsi, wsi->vhost->name, wsi->protocol->name,
+			 (unsigned long)len);
+		// assert(0);
+
+		return -1;
+	}
+
 	lws_stats_atomic_bump(wsi->context, pt, LWSSTATS_C_API_WRITE, 1);
 
 	if (!len)
@@ -62,13 +79,12 @@ int lws_issue_raw(struct lws *wsi, unsigned char *buf, size_t len)
 
 	if (wsi->trunc_len && (buf < wsi->trunc_alloc ||
 	    buf > (wsi->trunc_alloc + wsi->trunc_len + wsi->trunc_offset))) {
-		char dump[20];
-		strncpy(dump, (char *)buf, sizeof(dump) - 1);
-		dump[sizeof(dump) - 1] = '\0';
-		lwsl_err("** %p: Sending new %lu (%s), pending truncated ...\n"
+		lwsl_hexdump_level(LLL_ERR, buf, len);
+		lwsl_err("** %p: vh: %s, prot: %s, Sending new %lu, pending truncated ...\n"
 			 "   It's illegal to do an lws_write outside of\n"
 			 "   the writable callback: fix your code\n",
-			 wsi, (unsigned long)len, dump);
+			 wsi, wsi->vhost->name, wsi->protocol->name,
+			 (unsigned long)len);
 		assert(0);
 
 		return -1;
@@ -102,13 +118,20 @@ int lws_issue_raw(struct lws *wsi, unsigned char *buf, size_t len)
 	n = lws_ssl_capable_write(wsi, buf, n);
 	lws_latency(context, wsi, "send lws_issue_raw", n, n == len);
 
+	/* something got written, it can have been truncated now */
+	wsi->could_have_pending = 1;
+
 	switch (n) {
 	case LWS_SSL_CAPABLE_ERROR:
 		/* we're going to close, let close know sends aren't possible */
 		wsi->socket_is_permanently_unusable = 1;
 		return -1;
 	case LWS_SSL_CAPABLE_MORE_SERVICE:
-		/* nothing got sent, not fatal, retry the whole thing later */
+		/*
+		 * nothing got sent, not fatal.  Retry the whole thing later,
+		 * ie, implying treat it was a truncated send so it gets
+		 * retried
+		 */
 		n = 0;
 		break;
 	}
@@ -143,7 +166,7 @@ handle_truncated_send:
 
 	/*
 	 * Newly truncated send.  Buffer the remainder (it will get
-	 * first priority next time the socket is writable)
+	 * first priority next time the socket is writable).
 	 */
 	lwsl_debug("%p new partial sent %d from %lu total\n", wsi, n,
 		    (unsigned long)real_len);

lib/plat/lws-plat-esp32.c

@@ -79,6 +79,9 @@ lws_send_pipe_choked(struct lws *wsi)
 #endif
 	int n;
 
+	/* the fact we checked implies we avoided back-to-back writes */
+	wsi_eff->could_have_pending = 0;
+
 	/* treat the fact we got a truncated send pending as if we're choked */
 	if (wsi_eff->trunc_len)
 		return 1;

lib/plat/lws-plat-optee.c

@@ -45,6 +45,19 @@ lws_get_random(struct lws_context *context, void *buf, int len)
 LWS_VISIBLE int
 lws_send_pipe_choked(struct lws *wsi)
 {
+	struct lws *wsi_eff = wsi;
+
+#if defined(LWS_WITH_HTTP2)
+	wsi_eff = lws_get_network_wsi(wsi);
+#endif
+
+	/* the fact we checked implies we avoided back-to-back writes */
+	wsi_eff->could_have_pending = 0;
+
+	/* treat the fact we got a truncated send pending as if we're choked */
+	if (wsi_eff->trunc_len)
+		return 1;
+
 #if 0
 	struct lws_pollfd fds;
 

lib/plat/lws-plat-unix.c

@@ -93,6 +93,10 @@ lws_send_pipe_choked(struct lws *wsi)
 #if defined(LWS_WITH_HTTP2)
 	wsi_eff = lws_get_network_wsi(wsi);
 #endif
+
+	/* the fact we checked implies we avoided back-to-back writes */
+	wsi_eff->could_have_pending = 0;
+
 	/* treat the fact we got a truncated send pending as if we're choked */
 	if (wsi_eff->trunc_len)
 		return 1;

lib/plat/lws-plat-win.c

@@ -135,12 +135,19 @@ lws_get_random(struct lws_context *context, void *buf, int len)
 
 LWS_VISIBLE int
 lws_send_pipe_choked(struct lws *wsi)
-{
+{	struct lws *wsi_eff = wsi;
+
+#if defined(LWS_WITH_HTTP2)
+	wsi_eff = lws_get_network_wsi(wsi);
+#endif
+	/* the fact we checked implies we avoided back-to-back writes */
+	wsi_eff->could_have_pending = 0;
+
 	/* treat the fact we got a truncated send pending as if we're choked */
-	if (wsi->trunc_len)
+	if (wsi_eff->trunc_len)
 		return 1;
 
-	return (int)wsi->sock_send_blocking;
+	return (int)wsi_eff->sock_send_blocking;
 }
 
 LWS_VISIBLE int

lib/private-libwebsockets.h

@@ -515,6 +515,9 @@ enum lws_connection_states {
 							  _LSF_WEBSOCKET,
 
 	LWSS_CGI					= 17,
+
+	LWSS_HTTP2_DEFERRING_ACTION			= _LSF_CCB | 18 |
+							  _LSF_POLLOUT,
 };
 
 #define lws_state_is_ws(s) (!!(s & _LSF_WEBSOCKET))
@@ -1925,6 +1928,8 @@ struct lws {
 	unsigned int rxflow_will_be_applied:1;
 	unsigned int event_pipe:1;
 
+	unsigned int could_have_pending:1; /* detect back-to-back writes */
+
 	unsigned int timer_active:1;
 
 #ifdef LWS_WITH_ACCESS_LOG

lib/service.c

@@ -25,7 +25,7 @@ static int
 lws_calllback_as_writeable(struct lws *wsi)
 {
 	struct lws_context_per_thread *pt = &wsi->context->pt[(int)wsi->tsi];
-	int n;
+	int n, m;
 
 	lws_stats_atomic_bump(wsi->context, pt, LWSSTATS_C_WRITEABLE_CB, 1);
 #if defined(LWS_WITH_STATS)
@@ -62,9 +62,11 @@ lws_calllback_as_writeable(struct lws *wsi)
 		break;
 	}
 
-	return user_callback_handle_rxflow(wsi->protocol->callback,
+	m = user_callback_handle_rxflow(wsi->protocol->callback,
 					   wsi, (enum lws_callback_reasons) n,
 					   wsi->user_space, NULL, 0);
+
+	return m;
 }
 
 LWS_VISIBLE int
@@ -95,6 +97,7 @@ lws_handle_POLLOUT_event(struct lws *wsi, struct lws_pollfd *pollfd)
 	 *	       If anything else sent first the protocol would be
 	 *	       corrupted.
 	 */
+	wsi->could_have_pending = 0; /* clear back-to-back write detection */
 	if (wsi->trunc_len) {
 		//lwsl_notice("%s: completing partial\n", __func__);
 		if (lws_issue_raw(wsi, wsi->trunc_alloc + wsi->trunc_offset,
@@ -464,6 +467,38 @@ user_service_go_again:
 			goto next_child;
 		}
 
+		if (w->state == LWSS_HTTP2_DEFERRING_ACTION) {
+
+			/*
+			 * we had to defer the http_action to the POLLOUT
+			 * handler, because we know it will send something and
+			 * only in the POLLOUT handler do we know for sure
+			 * that there is no partial pending on the network wsi.
+			 */
+
+			w->state = LWSS_HTTP2_ESTABLISHED;
+
+			lwsl_info("  h2 action start...\n");
+			n = lws_http_action(w);
+			lwsl_info("  h2 action result %d "
+				  "(wsi->http.rx_content_remain %lld)\n",
+				  n, w->http.rx_content_remain);
+
+			/*
+			 * Commonly we only managed to start a larger transfer
+			 * that will complete asynchronously under its own wsi
+			 * states.  In those cases we will hear about
+			 * END_STREAM going out in the POLLOUT handler.
+			 */
+			if (n || w->h2.send_END_STREAM) {
+				lwsl_info("closing stream after h2 action\n");
+				lws_close_free_wsi(w, LWS_CLOSE_STATUS_NOSTATUS);
+				wa = &wsi->h2.child_list;
+			}
+
+			goto next_child;
+		}
+
 		if (w->state == LWSS_HTTP_ISSUING_FILE) {
 
 			((volatile struct lws *)w)->leave_pollout_active = 0;

plugins/protocol_lws_status.c

@@ -62,18 +62,15 @@ struct per_vhost_data__lws_status {
 static void
 trigger_resend(struct per_vhost_data__lws_status *vhd)
 {
-	struct per_session_data__lws_status *pss = vhd->live_pss_list;
-
-	while (pss) {
+	lws_start_foreach_ll(struct per_session_data__lws_status *, pss,
+			     vhd->live_pss_list) {
 		if (pss->walk == WALK_NONE) {
 			pss->subsequent = 0;
 			pss->walk_next = vhd->live_pss_list;
 			pss->walk = WALK_INITIAL;
 		} else
 			pss->changed_partway = 1;
-
-		pss = pss->next;
-	}
+	} lws_end_foreach_ll(pss, next);
 
 	lws_callback_on_writable_all_protocol(vhd->context, vhd->protocol);
 }
@@ -85,8 +82,7 @@ callback_lws_status(struct lws *wsi, enum lws_callback_reasons reason,
 		    void *user, void *in, size_t len)
 {
 	struct per_session_data__lws_status *pss =
-			(struct per_session_data__lws_status *)user,
-			*pss1, *pss2;
+			(struct per_session_data__lws_status *)user;
 	struct per_vhost_data__lws_status *vhd =
 			(struct per_vhost_data__lws_status *)
 			lws_protocol_vh_priv_get(lws_get_vhost(wsi),
@@ -123,6 +119,7 @@ callback_lws_status(struct lws *wsi, enum lws_callback_reasons reason,
 		strcpy(pss->user_agent, "unknown");
 		lws_hdr_copy(wsi, pss->user_agent, sizeof(pss->user_agent),
 			     WSI_TOKEN_HTTP_USER_AGENT);
+
 		trigger_resend(vhd);
 		break;
 
@@ -150,14 +147,14 @@ callback_lws_status(struct lws *wsi, enum lws_callback_reasons reason,
 			pss->subsequent = 1;
 
 			m = 0;
-			pss2 = vhd->live_pss_list;
-			while (pss2) {
+			lws_start_foreach_ll(struct per_session_data__lws_status *,
+					     pss2, vhd->live_pss_list) {
 				if (pss2 == pss->walk_next) {
 					m = 1;
 					break;
 				}
-				pss2 = pss2->next;
-			}
+			} lws_end_foreach_ll(pss2, next);
+
 			if (!m) {
 				/* our next guy went away */
 				pss->walk = WALK_FINAL;
@@ -181,6 +178,7 @@ walk_final:
 			n = LWS_WRITE_CONTINUATION;
 			p += sprintf(p, "]}");
 			if (pss->changed_partway) {
+				pss->changed_partway = 0;
 				pss->subsequent = 0;
 				pss->walk_next = vhd->live_pss_list;
 				pss->walk = WALK_INITIAL;
@@ -207,22 +205,15 @@ walk_final:
 		break;
 
 	case LWS_CALLBACK_CLOSED:
-		pss1 = vhd->live_pss_list;
-		pss2 = NULL;
-
-		while (pss1) {
-			if (pss1 == pss) {
-				if (pss2)
-					pss2->next = pss->next;
-				else
-					vhd->live_pss_list = pss->next;
-
+		// lwsl_debug("****** LWS_CALLBACK_CLOSED\n");
+		lws_start_foreach_llp(struct per_session_data__lws_status **,
+			ppss, vhd->live_pss_list) {
+			if (*ppss == pss) {
+				*ppss = pss->next;
 				break;
 			}
+		} lws_end_foreach_llp(ppss, next);
 
-			pss2 = pss1;
-			pss1 = pss1->next;
-		}
 		trigger_resend(vhd);
 		break;
 

test-apps/test-server-http.c

@@ -503,11 +503,9 @@ int callback_http(struct lws *wsi, enum lws_callback_reasons reason, void *user,
 		if (n < 0)
 			return 1;
 
-		n = lws_write(wsi, (unsigned char *)pss->result + LWS_PRE,
-			      pss->result_len, LWS_WRITE_HTTP);
-		if (n < 0)
-			return 1;
-		goto try_to_reuse;
+		lws_callback_on_writable(wsi);
+		break;
+
 	case LWS_CALLBACK_HTTP_DROP_PROTOCOL:
 		lwsl_debug("LWS_CALLBACK_HTTP_DROP_PROTOCOL\n");
 
@@ -526,7 +524,7 @@ int callback_http(struct lws *wsi, enum lws_callback_reasons reason, void *user,
 		if (pss->client_finished)
 			return -1;
 
-		if (!lws_get_child(wsi) && !pss->fop_fd)
+		if (!lws_get_child(wsi) && !pss->fop_fd && !pss->result_len)
 			goto try_to_reuse;
 
 #ifndef LWS_NO_CLIENT
@@ -561,6 +559,17 @@ int callback_http(struct lws *wsi, enum lws_callback_reasons reason, void *user,
 		/*
 		 * we can send more of whatever it is we were sending
 		 */
+
+		if (pss->result_len) {
+			/* the result from the form */
+			n = lws_write(wsi, (unsigned char *)pss->result + LWS_PRE,
+				      pss->result_len, LWS_WRITE_HTTP);
+			pss->result_len = 0;
+			if (n < 0)
+				return 1;
+			goto try_to_reuse;
+		}
+
 		sent = 0;
 		do {
 			/* we'd like the send this much */