httpd: re-check the byte that breaks a partial boundary match

stream_upload() matches the closing multipart boundary byte by byte.
When a byte breaks a partial match, the matched prefix goes to flash as
data, and so does the breaking byte, without being compared against the
start of the boundary.

The boundary starts with "\r\n--", so an image whose last byte is '\r'
never completes: that '\r' starts a match, the '\r' of the boundary
breaks it and is written as data, and the rest of the boundary no
longer matches. The browser waits for a reply that never comes.

Flush the matched prefix and look at the breaking byte again from the
start of the boundary. A boundary may not contain CR, so no longer
suffix of the matched prefix can start a new match.

httpd/httpd.c

@@ -531,9 +531,10 @@ uint8_t stream_upload(void)
 				memcpy(flash_buf + write_len, boundary, bindex);
 				write_len += bindex;
 				bindex = 0;
+			} else {
+				crc16_bank1(upload_settings.p + upload_settings.bptr);
+				flash_buf[write_len++] = upload_settings.p[upload_settings.bptr++];
 			}
-			crc16_bank1(upload_settings.p + upload_settings.bptr);
-			flash_buf[write_len++] = upload_settings.p[upload_settings.bptr++];
 			if (write_len >= FLASH_PAGE_SIZE) {
 				/* The staged image spans FIRMWARE_UPLOAD_START to twice that,
 				 * the span check_and_flash_update_image() reads back. Nothing
@@ -565,7 +566,6 @@ uint8_t stream_upload(void)
 					memcpy(flash_buf, flash_buf + FLASH_PAGE_SIZE, write_len);
 				}
 			}
-			bindex = 0;
 		}
 	} while(1);
 }

test/test_httpd_tx.c

@@ -81,7 +81,7 @@ static uint8_t written_config[CONFIG_LEN];
 static uint16_t written_config_len;
 static uint32_t config_erase_addr, config_write_addr;
 static int config_erases, config_writes;
-static uint8_t flash_written[64];
+static uint8_t flash_written[1024];
 static size_t flash_written_len;
 static int flash_write_bad;
 
@@ -551,7 +551,7 @@ static void scenario_bad_l4_checksum(void)
 
 
 /* Private HTTP lifecycle values; drive the real callback, not a model. */
-enum { TSTATE_NONE = 0, TSTATE_TX = 1 };
+enum { TSTATE_NONE = 0, TSTATE_TX = 1, TSTATE_POST = 4 };
 extern uint16_t slen, cont_len, sent_len, bindex, crc_final, write_len;
 extern uint8_t boundary[], verify_crc, outbuf[];
 extern struct { uint8_t *p; uint16_t bptr, plen; } upload_settings;
@@ -1084,6 +1084,69 @@ static void scenario_upload(const char *name, const char *part_type, int split)
 	flash_size = saved_flash_size;
 }
 
+extern uint32_t uptr;
+
+#define UPLOAD_LEN	600
+#define UPLOAD_BOUNDARY	"\r\n--XyZ"
+#define UPLOAD_TRAILER	UPLOAD_BOUNDARY "--\r\n"
+
+static int upload_in_two(uint8_t *body, uint16_t body_len, uint16_t split)
+{
+	uptr = FIRMWARE_UPLOAD_START;
+	write_len = 0;
+	bindex = 0;
+	verify_crc = 0;
+	flash_written_len = 0;
+	flash_write_bad = 0;
+	strcpy((char *)boundary, UPLOAD_BOUNDARY);
+	uip_conn = &uip_conns[0];
+	uip_conn->appstate.tstate = TSTATE_POST;
+
+	upload_settings.p = body;
+	upload_settings.bptr = 0;
+	upload_settings.plen = split;
+	stream_upload();
+	if (uip_conn->appstate.tstate == TSTATE_POST) {
+		upload_settings.p = body + split;
+		upload_settings.bptr = 0;
+		upload_settings.plen = body_len - split;
+		stream_upload();
+	}
+	return uip_conn->appstate.tstate == TSTATE_NONE
+		&& uptr == FIRMWARE_UPLOAD_START + UPLOAD_LEN && !flash_write_bad
+		&& flash_written_len == UPLOAD_LEN + 1 && !flash_written[UPLOAD_LEN];
+}
+
+static void scenario_upload_cr_before_boundary(void)
+{
+	uint8_t body[UPLOAD_LEN + sizeof(UPLOAD_TRAILER)];
+	uint16_t body_len, split;
+	int completed = 0, intact = 0, splits = 0;
+
+	for (uint16_t i = 0; i < UPLOAD_LEN; i++)
+		body[i] = pattern(i);
+	body[UPLOAD_LEN - 2] = 'x';
+	body[UPLOAD_LEN - 1] = '\r';
+	body_len = UPLOAD_LEN + sizeof(UPLOAD_TRAILER) - 1;
+	memcpy(body + UPLOAD_LEN, UPLOAD_TRAILER, sizeof(UPLOAD_TRAILER) - 1);
+
+	for (split = 0; split <= body_len; split++) {
+		splits++;
+		if (upload_in_two(body, body_len, split)) {
+			completed++;
+			if (!memcmp(flash_written, body, UPLOAD_LEN))
+				intact++;
+		}
+	}
+	if (verbose)
+		printf("      %d of %d splits complete, %d intact\n", completed, splits, intact);
+
+	CHECK(completed == splits,
+	      "upload: an image ending in CR still finds the closing boundary");
+	CHECK(intact == splits,
+	      "upload: the CR and the bytes before it reach the flash unchanged");
+}
+
 int main(int argc, char **argv)
 {
 	if (argc > 1 && !strcmp(argv[1], "-v"))
@@ -1115,6 +1178,7 @@ int main(int argc, char **argv)
 	scenario_upload("octet-stream control", "Content-Type: application/octet-stream\r\n", 0);
 	scenario_upload("MacBinary, split part terminator", "Content-Type: application/macbinary\r\n", 1);
 	scenario_upload("no part Content-Type", "", 0);
+	scenario_upload_cr_before_boundary();
 
 	printf("\n%s (%d failure%s)\n",
 	       failures ? "BENCH: FAILURES" : "BENCH: ALL PASS",