Bug Fixes - #40
Merged
Merged
Conversation
Two races identified by TSAN (ASAN and valgrind miss them) caused heap corruption that surfaced as NULL buffer->data in HTTP body handlers: 1. connection->sock use-after-free: _connection_close_fd (worker thread) freed the socket and set connection->sock = NULL while _connection_read_callback (I/O thread) read it. Fixed by making connection->sock an ATOMIC(platform_socket_t*) and deferring the socket's close+free to the I/O thread's destroy stack via http_server_defer_socket_destroy, mirroring the existing watcher/timer deferral. 2. pipe_notifiers use-after-free WRITE: readable_push_stream_pipe / writeable_pull_stream_pipe called on_pipe/on_piped synchronously on the caller's thread, writing pipe_notifiers while stream_unsubscribe_pipe_notifiers (worker thread) freed it. Fixed by routing pipe/piped through the stream actor via the already-declared STREAM_PIPE/STREAM_PIPED messages and stream_pipe_internal/stream_piped_internal. server_destroy_node_t generalized from is_timer bool to a type enum (0=watcher, 1=timer, 2=socket) to support deferred socket destruction. Verified: TestPushFileStream.*, TestPullFileStream.*, TestStreamActor.*, TestHttpServer.*, TestOffRoutes.*, TestHttpServerSsl.* pass under TSAN with zero data-race reports; full 849-test suite passes.
The get_pipeline_t refcount never reached zero, leaking 48 bytes (the struct) + 209 bytes indirect (the ori_t) per GET request. Root cause: refcounter_init + 4x refcounter_reference created 5 refs, but only 2-3 derefs happened. stream_deactivate emits both close_event and error_event, so desc contributes 1 or 2 derefs depending on path (normal close = 1, deactivate = 2), while rs contributes 1. The asymmetric deref count meant no path reached zero. Fix: add a desc_done flag so desc contributes exactly one deref (whichever of close or error fires first sets the flag and derefs; the other is a no-op). Reduce refs to init + 1x reference = 2 refs, matching the 2 derefs (desc-done + rs-done) in all paths. Verified: 7 TestOffRoutes tests pass under valgrind with 0 leaks (was: 48 bytes direct + 209 bytes indirect in _setup_stream_pipeline).
…n-flaky The test called srand(2) expecting rand() determinism, but store_block_should_accept draws from platform_random_uniform_float (CSPRNG via getentropy on Linux), which srand() cannot seed. Each run had a 75% chance of passing — observed 2-of-3 pass rate. Replaced the single-trial assertion with a 2000-trial statistical check: at capacity=0.2, INHALE phase, accept_probability=0.75, so expected accepts = 1500 ± 100 (≈5.2σ band, ~1-in-5M false-fail rate). Verified: 5/5 consecutive runs pass; full 849-test suite passes.
… ofd.js from bundle)
_pending_fetches nodes were never unlinked when a block result was consumed, so _skip_pending_tuple parked already-answered hashes into stale_fetches. For a block hash shared across tuples (recycler recipes) this permanently dropped the only remaining reply for a live tuple, wedging blocks_received below blocks_expected with no error. Answered fetches are now pruned at result-processing time in both the cache accumulate path and the network direct-return path, so stale_fetches only holds strictly unanswered hashes and matching late results by hash stays one-to-one (documented in _consume_stale_fetch). Also: tally/notify the load count before the render that can emit finished/complete/close, destroy xor_accumulator in readable_off_stream_destroy, reset state to OFF_STREAM_FETCHING_BLOCKS after a skip, and harden the test recorders (no terminate on extra events, load-before-close assertions, loaded+skipped == tuples written, shared-hash-across-skip regression test).
LOAD_REQUEST (39) now mirrors its GET sibling on the unix socket, WS and TCP transports: an authenticated request builds an ORI from the parsed OFF URL, runs a load-mode readable_off_stream (+ readable_descriptor) and reports progress as LOAD_PROGRESS (40) frames terminated by exactly one LOAD_END (41) guarded by terminal_sent. Status is FAILED on desc/rs error_event or no tuples loaded, PARTIAL when any tuple was skipped, and LOADED otherwise; counters are tracked on the pipeline from load_tuple_event payloads, never read from stream fields. - tuple totals computed via the new shared off_block_size_for_type() helper on readable_off_stream (body moved out of _block_size_for_type) - unix connections pass the network actor (peer_ctx.network) so LOAD can pull remote blocks; WS/TCP connections have no network access and run cache-only (network == NULL is a legal load mode) - directory (OFD) ORIs are rejected with BAD_REQUEST for v1 - directories resolve in HTTP land; extend later if asked - has_range/range_start/range_end are ignored on LOAD, matching the unix GET handler which ignores them today - TDD: TestUnixTransport.LoadRoundTrip (PUT 2-tuple file, LOAD, expect 2 progress frames + LOAD_END LOADED with correct counts) observed failing without the dispatch and passing with it; test flips the client socket nonblocking during the LOAD read phase so a missing frame cannot stall teardown on a blocking recv
GET ...?load=1 (or bare ?load) on the OFF GET route now runs the cache-load pipeline instead of serving file data: an off-stream in load mode pulls the file's blocks into the block cache and the response streams one application/x-ndjson progress line per tuple plus exactly one terminal line (status loaded/partial/failed). Directory URLs are rejected 400 (v1 parity with the socket LOAD handler); Range is honored like the data path, with a trimmed tuple total and 416 for unsatisfiable ranges. The response body's length only settles when the load ends, so the response is close-delimited: http_response_t gains an unknown_length flag that suppresses the auto Content-Length stamp in _send_headers, and the load pipeline forces keep_alive=0 and mirrors http_response_pipe's response/connection lifetime management at the terminal. Pipeline refcount discipline mirrors get_pipeline_t (base + one ref split across rs-done and desc_done-guarded desc-done); the terminal state machine mirrors the unix _unix_load_pipeline_t. Error handlers must NOT re-call stream_deactivate on an already-deactivated stream: stream_deactivate re-notifies error_event unconditionally, which would loop forever.
… ofd.js from bundle)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug Fixes