From 714c9eb34340786dd4df1bfcc305434b4affa088 Mon Sep 17 00:00:00 2001 From: DrDoof <8052722+DrDoof@users.noreply.github.com> Date: Sat, 10 Oct 2026 17:34:57 +0200 Subject: [PATCH 1/2] 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 | 6 ++-- test/test_httpd_tx.c | 68 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/httpd/httpd.c b/httpd/httpd.c index 688513f8..38bd5b27 100644 --- a/httpd/httpd.c +++ b/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); } diff --git a/test/test_httpd_tx.c b/test/test_httpd_tx.c index 0397e3de..a78091c4 100644 --- a/test/test_httpd_tx.c +++ b/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", From 241edfdbda2f0b769953fd5f9a90e512a219f5c8 Mon Sep 17 00:00:00 2001 From: d00f <8052722+DrDoof@users.noreply.github.com> Date: Thu, 22 Oct 2026 16:17:47 +0200 Subject: [PATCH 2/2] test: every last byte, CR runs and broken matches through the upload Three more bench scenarios around the closing boundary. Every value of the last image byte, uploaded whole and split right after the payload, must end the part. A run of CR before the boundary of even length passed before the fix and an odd one did not, so both are checked. An image made of CR only, longer than a flash page, and one made of repeated "\r\n-" exercise the broken matches back to back: each one appends to the page buffer without a byte written, and the page flush after both paths has to keep up. --- test/test_httpd_tx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/test/test_httpd_tx.c b/test/test_httpd_tx.c index a78091c4..99681fe5 100644 --- a/test/test_httpd_tx.c +++ b/test/test_httpd_tx.c @@ -1147,6 +1147,52 @@ static void scenario_upload_cr_before_boundary(void) "upload: the CR and the bytes before it reach the flash unchanged"); } +static int upload_whole_and_split(uint8_t *body, uint16_t body_len) +{ + return upload_in_two(body, body_len, body_len) && !memcmp(flash_written, body, UPLOAD_LEN) + && upload_in_two(body, body_len, UPLOAD_LEN) && !memcmp(flash_written, body, UPLOAD_LEN); +} + +static void scenario_upload_last_byte(void) +{ + uint8_t body[UPLOAD_LEN + sizeof(UPLOAD_TRAILER)]; + uint16_t body_len = UPLOAD_LEN + sizeof(UPLOAD_TRAILER) - 1; + int bad = -1; + + for (uint16_t i = 0; i < UPLOAD_LEN; i++) + body[i] = pattern(i); + memcpy(body + UPLOAD_LEN, UPLOAD_TRAILER, sizeof(UPLOAD_TRAILER) - 1); + for (int last = 0; last < 256 && bad < 0; last++) { + body[UPLOAD_LEN - 1] = last; + if (!upload_whole_and_split(body, body_len)) + bad = last; + } + if (bad >= 0) + printf(" last byte 0x%02x loses the closing boundary\n", bad); + CHECK(bad < 0, "upload: the part ends after the payload whatever its last byte"); + + body[UPLOAD_LEN - 1] = '\r'; + body[UPLOAD_LEN - 2] = '\r'; + CHECK(upload_whole_and_split(body, body_len), "upload: an even run of CR before the boundary"); + body[UPLOAD_LEN - 3] = '\r'; + CHECK(upload_whole_and_split(body, body_len), "upload: an odd run of CR before the boundary"); +} + +static void scenario_upload_broken_matches(void) +{ + uint8_t body[UPLOAD_LEN + sizeof(UPLOAD_TRAILER)]; + uint16_t body_len = UPLOAD_LEN + sizeof(UPLOAD_TRAILER) - 1; + + memcpy(body + UPLOAD_LEN, UPLOAD_TRAILER, sizeof(UPLOAD_TRAILER) - 1); + memset(body, '\r', UPLOAD_LEN); + CHECK(upload_whole_and_split(body, body_len), + "upload: a run of CR longer than a flash page is staged intact"); + for (uint16_t i = 0; i < UPLOAD_LEN; i++) + body[i] = "\r\n-"[i % 3]; + CHECK(upload_whole_and_split(body, body_len), + "upload: repeated partial delimiters are staged as payload"); +} + int main(int argc, char **argv) { if (argc > 1 && !strcmp(argv[1], "-v")) @@ -1179,6 +1225,8 @@ int main(int argc, char **argv) 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(); + scenario_upload_last_byte(); + scenario_upload_broken_matches(); printf("\n%s (%d failure%s)\n", failures ? "BENCH: FAILURES" : "BENCH: ALL PASS",