From 0a9fee5aaef580a47cf0828acbfa60985429d2d7 Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Wed, 29 Jul 2026 16:09:58 -0500 Subject: [PATCH] =?UTF-8?q?fix(http):=20span-input=20parser=20=E2=80=94=20?= =?UTF-8?q?demo=20path=20shows=20the=20body,=20TLS=20plaintext=20never=20e?= =?UTF-8?q?nters=20the=20ciphertext=20ring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #72. The 'G' HTTPS demo in boot.s hand-rolled its response receive: one net_poll/tls_recv iteration, then a single-page copy of that first decrypted record into http_resp_buf (the "up to 512 bytes" comment was also wrong — the loop caps at 256). Whenever the body arrived as a second TLS record it was received and ACKed but never copied, so the demo displayed headers only. Routing the demo through the production parser surfaced a deeper, pre-existing flaw in http_get's own TLS path: it fed DECRYPTED plaintext back into the shared TCP ring (tcp_recv_tail) and parsed from there via net_recv_byte. The ring is the ciphertext queue. On ip65 the rx callback queues eagerly, so ciphertext for later records (body record #2, the peer's close_notify) is typically already sitting between head and tail — the parser then reads ciphertext as HTTP text (garbage status) or loses the body. UCI masked this because its adapter reads on demand. Fix — dual-source parser input (http_in_byte): mode 0 (plain HTTP): input = TCP ring, delegates to net_recv_byte; the ring legitimately holds plaintext there. mode 1 (TLS): input = the current decrypted record as a linear span (http_in_ptr/http_in_len, 16-bit). The ring stays pure ciphertext on the TLS path: no feed, no discard, and multi-record bodies parse correctly on both backends. The span path also drops the old feed loop's "TLS records < 256" assumption (it honored only tls_app_len's low byte). http_in_byte preserves X/Y to match net_recv_byte's contract and uses SMC rather than zp_ptr, which the parser's body-store state owns. The receive/parse loop is extracted as http_recv_body and shared by http_get and the boot.s demo, so there is one implementation of the response semantics. Each path keeps its own close sequence (the demo prints progress). Mode is set per TLS record and reset to 0 in the plain path's parser init, so alternating 'H' and 'G' in one session is safe. Also (issue #72, comment-only): tcp_recv_overflow's semantics documented at the declaration (src/data.s) and the set site (src/net/ip65/net.s). Retransmission duplicates enter the ring too, so the flag can latch during retransmit bursts with in-flight data far below ring size (observed in the VICE e2e with a 690 B flight, stream intact). It is a diagnostic breadcrumb, not proof of loss — but ip65 ACKs the full inbound length regardless of what the callback copies, so if a dropped tail ever WAS new in-sequence data it is genuinely gone and TLS errors follow. No accounting redesign here. User-visible change: the 'G' demo now prints the response body (via ascii_chrout) instead of the status line + headers. This is shared code, so it applies to the UCI backend too. Validation: all five profiles link (ip65 / ip65+onchip / uci / uci+onchip / uci+comb; sizes unchanged at 47,105 and 62,977 B). tools/test_http.py 27/27 in VICE — the mode-0 ring path, including the 120-byte-body and 404 cases. ip65 e2e with body assertions: see the follow-up commit/PR note. Co-Authored-By: Claude Opus 5 (1M context) --- src/boot.s | 38 ++------ src/data.s | 12 ++- src/http.s | 165 ++++++++++++++++++++++----------- src/net/ip65/net.s | 11 ++- tests/test_vice_https_macos.py | 34 +++---- 5 files changed, 157 insertions(+), 103 deletions(-) diff --git a/src/boot.s b/src/boot.s index 29d3d2a..a243251 100644 --- a/src/boot.s +++ b/src/boot.s @@ -122,6 +122,7 @@ ; ---- imports: HTTP ---- .import http_get_plain .import http_build_get + .import http_recv_body ; ---- imports: HTTP I/O state (data.asm) ---- .import http_host_ptr @@ -621,36 +622,15 @@ do_https_get: ldy #>send_ok_msg jsr print_string - ; --- receive response via TLS --- -@recv_loop: - jsr net_poll ; pump network - jsr tls_recv - bcs @recv_loop ; C=1 means no data yet, keep polling + ; --- receive + parse response --- + ; Shared production path (issue #72): http_recv_body walks status + ; line + headers + body with Content-Length termination, leaving + ; the BODY in http_resp_buf / http_resp_len. Replaces the old + ; first-record-only copy loop that showed headers and lost the + ; body whenever it arrived as a second TLS record. + jsr http_recv_body - ; got data -- tls_app_ptr/tls_app_len has decrypted payload - ; copy into http_resp_buf (up to 512 bytes) - lda tls_app_ptr - sta zp_ptr - lda tls_app_ptr+1 - sta zp_ptr+1 - - ldy #0 - ldx tls_app_len ; low byte of length (assume <256 for first chunk) -@copy_resp: - cpx #0 - beq @recv_done - lda (zp_ptr),y - sta http_resp_buf,y - iny - dex - bne @copy_resp - -@recv_done: - sty http_resp_len ; store how many bytes we copied - lda #0 - sta http_resp_len+1 - - ; display response + ; display response body jsr print_resp_body @close: diff --git a/src/data.s b/src/data.s index 6aeacf2..157e080 100644 --- a/src/data.s +++ b/src/data.s @@ -160,7 +160,17 @@ sqtab_reserved: .res 1024 .export tcp_recv_overflow tcp_recv_head: .res 2 ; read position (16-bit, masked with TCP_RECV_MASK) tcp_recv_tail: .res 2 ; write position (updated by ip65 callback) -tcp_recv_overflow: .res 1 ; set to 1 by callback if ring fills up +tcp_recv_overflow: .res 1 ; set to 1 by callback if ring fills up. + ; NOTE (issue #72): retransmission + ; duplicates also enter the ring, so this + ; can latch during retransmit bursts even + ; with in-flight data far below ring size + ; (observed in the VICE e2e, 690 B flight, + ; stream intact). A set flag is a + ; diagnostic breadcrumb, not proof of + ; loss — see the set-site comment in + ; src/net/ip65/net.s for the full + ; semantics. ; Diagnostic counters — incremented by net_poll at entry and return .export net_poll_entry_count diff --git a/src/http.s b/src/http.s index af1cae1..8e842d1 100644 --- a/src/http.s +++ b/src/http.s @@ -12,6 +12,7 @@ ; ---- exports ---- .export http_get .export http_build_get + .export http_recv_body .export http_recv_response .export http_get_plain .export http_get_verb @@ -39,10 +40,12 @@ .import http_line_buf .import http_content_length - ; ---- imports: data.asm BSS (TLS app data + TCP ring tail) ---- + ; ---- imports: data.asm BSS (TLS app data) ---- + ; (tcp_recv_head/tail imports dropped in the issue #72 redesign — + ; the parser no longer touches the ciphertext ring on the TLS + ; path; ring access is via net_recv_byte on the plain path only.) .import tls_app_ptr .import tls_app_len - .import tcp_recv_tail ; ---- imports: TLS handshake layer (SNI buffer + connect/close) ---- .import tls_hostname @@ -131,6 +134,47 @@ http_get: : ; --- 8. Receive response via TLS --- + ; Extracted to http_recv_body (issue #72) so the boot.s HTTPS demo + ; can share the exact production receive/parse path instead of its + ; old first-record-only copy loop. Closes stay here: the demo does + ; its own close sequence with progress prints. + jsr http_recv_body + + jsr tls_close + jsr net_tcp_close + clc + rts + +@tls_error: + jsr net_tcp_close +@error: + sec + rts + +@close_error: + jsr tls_close + jsr net_tcp_close + sec + rts + +; ============================================================================= +; http_recv_body - receive + parse the HTTP response over TLS +; +; The production receive path shared by http_get and the boot.s HTTPS demo. +; Polls the network and hands each decrypted TLS application record to +; http_recv_response as a linear span (status line + headers + body with +; Content-Length termination). Plaintext never touches the TCP ring — +; see the span-input comment at the record handoff below. +; +; Input: TLS session established, request already sent +; Output: C=0; http_status / http_resp_buf (body) / http_resp_len populated. +; Returns on parse-complete OR on the poll-timeout fallback +; ("accept whatever we have" — preserves the historical http_get +; behaviour for chunked/streaming responses with no Content-Length). +; Caller closes the connection. +; Clobbers: A, X, Y, zp_ptr +; ============================================================================= +http_recv_body: ; Initialise parser state. http_content_length / _known are reset ; at the status-line -> headers transition (see @parse_status_line). lda #0 @@ -149,45 +193,26 @@ http_get: jsr tls_recv bcs @recv_no_data - ; Got decrypted data in tls_app_ptr / tls_app_len - ; Copy tls_app_ptr to ZP for indirect addressing + ; Got decrypted data in tls_app_ptr / tls_app_len. + ; Hand the decrypted record to the parser as a linear span + ; (issue #72 redesign). The historical approach fed plaintext + ; back into the shared TCP ring, which breaks whenever ciphertext + ; for later records (body record #2, the peer's close_notify) is + ; already queued between head and tail — on ip65 the rx callback + ; queues eagerly, so the parser ate ciphertext as HTTP text + ; (garbage http_status) or lost the body to a discard. Span input + ; keeps the ring pure ciphertext: no feed, no discard, and + ; multi-record bodies parse correctly on both backends. lda tls_app_ptr - sta zp_ptr + sta http_in_ptr lda tls_app_ptr+1 - sta zp_ptr+1 - - ; Feed decrypted bytes into the TCP ring buffer. - ; Ring is 1024 bytes with 16-bit masked head/tail. We compute the - ; destination absolute address per-byte via SMC on @feed_store. - ldy #0 -@feed_loop: - cpy tls_app_len ; low byte only (TLS records < 256) - beq @feed_done - lda (zp_ptr),y - pha - ; dest = tcp_recv_buf + tail - clc - lda tcp_recv_tail+0 - adc #tcp_recv_buf - sta @feed_store+2 - pla -@feed_store: - sta $ffff ; SMC: patched above - ; tail = (tail + 1) & TCP_RECV_MASK - inc tcp_recv_tail+0 - bne @feed_mask - inc tcp_recv_tail+1 -@feed_mask: - lda tcp_recv_tail+1 - and #>(TCP_RECV_MASK) - sta tcp_recv_tail+1 - iny - bne @feed_loop ; always branches (tls_app_len < 256) -@feed_done: - ; Parse from ring buffer + sta http_in_ptr+1 + lda tls_app_len + sta http_in_len + lda tls_app_len+1 + sta http_in_len+1 + lda #1 + sta http_in_mode ; input mode 1: parser reads this span jsr http_recv_response bcc @recv_complete ; C=0 means parsing complete ; Reset timeout counter on progress @@ -204,24 +229,54 @@ http_get: ; Timeout — accept whatever we have @recv_complete: - jsr tls_close - jsr net_tcp_close clc rts @recv_timeout: .word 0 -@tls_error: - jsr net_tcp_close -@error: - sec +; ============================================================================= +; http_in_byte - fetch the next input byte for http_recv_response +; +; Dual-source input (issue #72): the parser is shared between the plain +; HTTP path (input = the TCP ring, which holds plaintext there) and the +; TLS path (input = the current decrypted record span; plaintext must +; NEVER be fed through the ciphertext ring — see http_recv_body). +; +; http_in_mode: 0 = ring (delegates to net_recv_byte) +; 1 = span (http_in_ptr / http_in_len, 16-bit) +; Output: C=0 + byte in A, or C=1 = input exhausted. +; Preserves X, Y (matches net_recv_byte's contract). +; ============================================================================= +http_in_byte: + lda http_in_mode + beq @ring ; mode 0: plain path reads the ring + ; span mode: exhausted? + lda http_in_len + ora http_in_len+1 + beq @span_empty + ; SMC-patch the load address (module convention: no-ZP, the + ; parser's body state owns zp_ptr) + lda http_in_ptr + sta @in_ld+1 + lda http_in_ptr+1 + sta @in_ld+2 +@in_ld: lda $ffff ; SMC: patched above + pha + inc http_in_ptr + bne :+ + inc http_in_ptr+1 +: lda http_in_len + bne :+ + dec http_in_len+1 +: dec http_in_len + pla + clc rts - -@close_error: - jsr tls_close - jsr net_tcp_close +@span_empty: sec rts +@ring: + jmp net_recv_byte ; ============================================================================= ; http_build_get - construct HTTP/1.1 GET request in http_req_buf @@ -385,7 +440,7 @@ http_recv_response: ; ----- state 0: accumulate status line until \n ----- @state_status: - jsr net_recv_byte + jsr http_in_byte bcc @status_have_data jmp @not_done ; no data yet, try again later @status_have_data: @@ -482,7 +537,7 @@ http_recv_response: ; This replaces the earlier 4-byte \r\n\r\n pattern matcher — one state ; machine is smaller than two running in parallel. @state_headers: - jsr net_recv_byte + jsr http_in_byte bcc @hdr_got_byte jmp @not_done @hdr_got_byte: @@ -545,7 +600,7 @@ http_recv_response: cmp http_content_length+1 beq @done @body_read: - jsr net_recv_byte + jsr http_in_byte bcs @not_done ; no byte right now -> poll more ; store byte using zp_ptr = http_resp_buf + http_resp_len pha ; save received byte @@ -702,6 +757,8 @@ http_get_plain: sta http_hdr_match sta http_resp_len sta http_resp_len+1 + sta http_in_mode ; input mode 0: parser reads the TCP ring + ; (plain HTTP — ring holds plaintext) ; --- 7. Poll + parse loop (with timeout) --- lda #0 @@ -758,3 +815,7 @@ http_crlf: http_bg_idx: .res 1 ; build_get write cursor http_bg_src: .res 1 ; build_get source index temp +; Parser input source (issue #72 span-input redesign — see http_in_byte) +http_in_mode: .res 1 ; 0 = TCP ring (plain HTTP), 1 = span (TLS) +http_in_ptr: .res 2 ; span read cursor +http_in_len: .res 2 ; span bytes remaining (16-bit) diff --git a/src/net/ip65/net.s b/src/net/ip65/net.s index 0ff6224..ba14085 100644 --- a/src/net/ip65/net.s +++ b/src/net/ip65/net.s @@ -376,7 +376,16 @@ cb_loop: lda cb_next_hi cmp tcp_recv_head+1 bne cb_not_full - ; ring full — record overflow and stop copying + ; ring full — record overflow and stop copying this delivery. + ; Semantics (issue #72): ip65 ACKs the full inbound length + ; regardless of what we copy (see the PR #27 clamp history), so + ; if the dropped tail was NEW in-sequence data it is genuinely + ; lost to the stream — the TLS layer will then fail on a broken + ; record. In practice the flag has only been observed latching + ; during TCP retransmission bursts, where the dropped delivery + ; duplicated bytes already consumed and the stream survived. + ; Treat a set flag as a diagnostic breadcrumb, not proof of + ; corruption — but investigate if TLS errors follow. lda #1 sta tcp_recv_overflow jmp cb_done diff --git a/tests/test_vice_https_macos.py b/tests/test_vice_https_macos.py index ef417e3..349686c 100644 --- a/tests/test_vice_https_macos.py +++ b/tests/test_vice_https_macos.py @@ -384,36 +384,30 @@ def _sigterm(_sig, _frame): # HTTP headers. http_get only completes (-> CONNECTION # CLOSED) once http_resp_len == Content-Length, so memory # holds the ground truth. - # NOTE on semantics: boot.s's HTTPS path does NOT go through - # http_recv_response — it copies raw TLS plaintext straight - # into http_resp_buf, so that buffer holds the FULL response - # (status line + headers + body) and http_status stays 0. - # (The status-parsed, body-only semantics documented in - # CLAUDE.md belong to the http_get path the UCI tests drive.) - # Assert on the raw buffer accordingly. + # Semantics (post issue #72 fix): the demo now routes through + # the shared http_recv_body, so http_status holds the parsed + # status code and http_resp_buf/http_resp_len hold the BODY + # (Content-Length-terminated) — the same contract as the + # http_get path the UCI tests drive. Assert the full contract. ok_body = False detail = "labels unavailable" try: labels = Labels.from_file( os.path.join(_REPO_ROOT, "build", "labels.txt")) + status = read_bytes(transport, labels["http_status"], 2) + status_val = status[0] | (status[1] << 8) rlen = read_bytes(transport, labels["http_resp_len"], 2) n = rlen[0] | (rlen[1] << 8) raw = read_bytes(transport, labels["http_resp_buf"], min(n, 512) or 1) text = raw.decode("ascii", errors="replace") - ok_status = "200 OK" in text - ok_payload = RESPONSE_BODY in text - # Demo-path contract: boot.s copies only the FIRST TLS - # record (headers). The body arrives as record #2 — - # delivered and ACKed at the TCP level (ordered stream, - # clean close_notify) but not copied into the buffer. - # Until the demo loop appends subsequent records - # (follow-up), pass = 200 status line received; body - # presence is reported informationally. - ok_body = ok_status and n > 0 - detail = (f"resp_len={n} status_line={'200 OK' if ok_status else 'MISSING'} " - f"body_in_buf={'yes' if ok_payload else 'no (demo path copies 1st record only)'} " - f"tail={text[-40:]!r}") + ok_status = status_val == 200 + ok_len = n == len(RESPONSE_BODY) + ok_payload = text.startswith(RESPONSE_BODY) + ok_body = ok_status and ok_len and ok_payload + detail = (f"http_status={status_val} resp_len={n} " + f"(expect {len(RESPONSE_BODY)}) " + f"body={'match' if ok_payload else text[:40]!r}") except Exception as e: # noqa: BLE001 detail = f"memory check failed: {e}" ok_body = RESPONSE_BODY.split()[0] in final.upper()