Merge pull request #582 from avicarmeli/fix/http-upload-reset-owner

Fix firmware upload reset ownership across HTTP connections

httpd/httpd.c

@@ -75,9 +75,6 @@ __xdata uint8_t postbody_endpoint;
 __xdata uint16_t postbody_start;
 
 __xdata char passwd[21];
-// Set when a verified firmware upload awaits its response ACK, after
-// which the chip resets to apply the staged image
-__xdata uint8_t fw_reset_pending;
 __xdata char session_id[SESSION_ID_LENGTH + 1];
 __xdata uint8_t authenticated;
 __xdata uint32_t now;
@@ -112,7 +109,6 @@ void httpd_init(void) __banked
 	// handling one, and nothing has set it yet at init time.
 	for (uint8_t i = 0; i < UIP_CONNS; i++)
 		uip_conns[i].appstate.tstate = TSTATE_CLOSED;
-	fw_reset_pending = 0; // xdata is not zeroed by the startup code
 }
 
 
@@ -513,7 +509,7 @@ uint8_t stream_upload(void)
 						"Content-Type: text/plain\r\n\r\n"
 						"OK: checksum verified, rebooting\n");
 					// Reset once the response is fully ACKed
-					fw_reset_pending = 1;
+					s->fw_reset_pending = 1;
 				} else {
 					print_string("Checksum incorrect! Aborting.\n");
 					slen = strtox(outbuf, "HTTP/1.1 400 Bad Request\r\nContent-Length: 33\r\n"
@@ -921,6 +917,7 @@ void httpd_appcall(void)
 	if(uip_connected()) {
 		dbg_string("Connected...\n");
 		s->tstate = TSTATE_NONE;
+		s->fw_reset_pending = 0; // explicitly initialize reused XDATA appstate
 	} else if (uip_closed()) {
 		dbg_string("Connection closed\n");
 		s->tstate = TSTATE_CLOSED;
@@ -972,7 +969,7 @@ void httpd_appcall(void)
 			cont_len -= slen;
 			cont_addr += slen;
 			s->tstate = TSTATE_TX;
-		} else if (fw_reset_pending) {
+		} else if (s->fw_reset_pending) {
 			// The upload verdict has been fully ACKed by the client;
 			// now it is safe to reset and apply the staged image
 			print_string("Resetting to apply update\n");

httpd/httpd.h

@@ -12,6 +12,7 @@
    for each TCP connection. */
 typedef struct httpd_state {
    uint8_t tstate;
+   uint8_t fw_reset_pending;
 } uip_tcp_appstate_t;
 
 /* Finally we define the application function to be called by uIP. */

test/test_httpd_tx.c

@@ -115,7 +115,8 @@ void flash_write_bytes(uint8_t *ptr)
 }
 const char *get_flash_size_str(void) { return "512 kB"; }
 void crc16_bank1(uint8_t *v) { (void)v; }
-void reset_chip(void) { }
+static unsigned reset_calls;
+void reset_chip(void) { reset_calls++; }
 void delay(uint16_t t) { (void)t; }
 void write_char(char c) { (void)c; }
 void write_char_no_syslog(char c) { (void)c; }
@@ -533,6 +534,120 @@ 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 };
+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;
+extern uint8_t stream_upload(void);
+
+static void scenario_upload_reset_owner(void)
+{
+	uint8_t byte = 0;
+	uint8_t get[] = "GET " FILE_NAME " HTTP/1.1\r\nHost: sw\r\n\r\n";
+	int initialized = 1;
+
+	/* The firmware has one TCP slot. Its appstate can contain stale bytes. */
+	uip_conn = &uip_conns[0];
+	reset_calls = 0;
+	for (unsigned state = 0; state <= 0xff; state++) {
+		uip_conn->appstate.tstate = state;
+		uip_conn->appstate.fw_reset_pending = 0xaa;
+		uip_flags = UIP_CONNECTED;
+		httpd_appcall();
+		if (uip_conn->appstate.tstate != TSTATE_NONE
+		    || uip_conn->appstate.fw_reset_pending != 0)
+			initialized = 0;
+	}
+	CHECK(initialized, "connect initializes both fields for every stale state");
+	CHECK(reset_calls == 0, "connection initialization does not reset");
+
+	/* End-of-part verdict with flash/CRC hardware edges mocked. */
+	boundary[0] = 0;
+	bindex = 0;
+	verify_crc = 1;
+	crc_final = 0xb001;
+	write_len = 0;
+	upload_settings.p = &byte;
+	upload_settings.bptr = 0;
+	upload_settings.plen = 1;
+	stream_upload();
+	CHECK(uip_conn->appstate.fw_reset_pending == 1,
+	      "verified upload records reset intent in its slot");
+
+	uip_conn->appstate.tstate = TSTATE_TX;
+	uip_conn->mss = MSS_FULL;
+	slen = 10;
+	sent_len = 5;
+	cont_len = 0;
+	uip_flags = UIP_ACKDATA;
+	httpd_appcall();
+	CHECK(reset_calls == 0 && slen == 5 && sent_len == 5,
+	      "partial verdict ACK sends the remainder without resetting");
+	uip_flags = UIP_ACKDATA;
+	httpd_appcall();
+	CHECK(reset_calls == 1 && slen == 0,
+	      "final verdict ACK resets after all response bytes are acknowledged");
+
+	/* No verdict ACK: a later connection reuses the same slot. */
+	reset_calls = 0;
+	uip_conn->appstate.tstate = TSTATE_TX;
+	uip_conn->appstate.fw_reset_pending = 1;
+	uip_flags = UIP_CONNECTED;
+	httpd_appcall();
+	CHECK(uip_conn->appstate.tstate == TSTATE_NONE
+	      && uip_conn->appstate.fw_reset_pending == 0,
+	      "slot reuse clears an unacknowledged upload's reset intent");
+	uip_appdata = get;
+	uip_len = strlen((char *)get);
+	uip_flags = UIP_NEWDATA;
+	httpd_appcall();
+	CHECK(uip_conn->appstate.tstate == TSTATE_TX && slen > 0,
+	      "reused slot serves a subsequent plain GET");
+	for (unsigned i = 0; i < 20 && uip_conn->appstate.tstate == TSTATE_TX; i++) {
+		uip_flags = UIP_ACKDATA;
+		httpd_appcall();
+	}
+	CHECK(reset_calls == 0 && slen == 0 && cont_len == 0,
+	      "subsequent GET response ACKs cannot inherit reset intent");
+
+	for (unsigned i = 0; i < 2; i++) {
+		uip_conn->appstate.tstate = TSTATE_TX;
+		uip_conn->appstate.fw_reset_pending = 1;
+		reset_calls = 0;
+		uip_flags = i ? UIP_TIMEDOUT : UIP_ABORT;
+		httpd_appcall();
+		CHECK(reset_calls == 0, "abort/timeout does not reset");
+		uip_flags = UIP_CONNECTED;
+		httpd_appcall();
+		CHECK(uip_conn->appstate.tstate == TSTATE_NONE
+		      && uip_conn->appstate.fw_reset_pending == 0,
+		      "connect after abort/timeout clears old upload intent");
+		uip_conn->appstate.tstate = TSTATE_TX;
+		slen = 0;
+		cont_len = 0;
+		sent_len = 0;
+		uip_flags = UIP_ACKDATA;
+		httpd_appcall();
+		CHECK(reset_calls == 0, "new connection ACK cannot inherit reset");
+	}
+
+	crc_final = 0;
+	bindex = 0;
+	stream_upload();
+	CHECK(uip_conn->appstate.fw_reset_pending == 0,
+	      "failed CRC does not request reset");
+	CHECK(strstr((char *)outbuf, "HTTP/1.1 400 Bad Request") != NULL,
+	      "failed CRC returns an error verdict");
+	uip_conn->appstate.tstate = TSTATE_TX;
+	sent_len = slen;
+	cont_len = 0;
+	uip_flags = UIP_ACKDATA;
+	httpd_appcall();
+	CHECK(reset_calls == 0, "failed CRC verdict ACK does not reset");
+
+}
+
 static void scenario_fragmented_post_header(void)
 {
 	static const char first[] =
@@ -908,6 +1023,7 @@ int main(int argc, char **argv)
 	scenario_growing_window();
 	scenario_rexmit_after_shrink();
 	scenario_bad_l4_checksum();
+	scenario_upload_reset_owner();
 	scenario_fragmented_post_header();
 	scenario_split_content_type();
 	scenario_split_header_terminator();