Repository navigation
fix: guard HttpSys SendResponse against a nil custom-header dict - #629
Merged
viniciussanchez merged 28 commits intoNov 9, 2026
Merged
viniciussanchez merged 28 commits into
viniciussanchez merged 28 commits into
Conversation
Takes upstream's Horse.Provider.FPC.HTTPApplication.pas wholesale. The conflict was entirely our inert PATCH-FPCHTTP-2: it hooked TFPCustomHttpServer's OnAllowConnect, which fphttpserver stores in its setter and reads nowhere, so the handler never fired and no setsockopt ever ran. Upstream's PR HashLoad#563 fixes the same Nagle + delayed-ACK stall with a THorseNoDelaySocketHandler.Accept override wired through OnGetSocketHandler, running once per connection rather than once per request, and additionally sets KeepConnectionTimeout - without which KeepConnections alone leaves the connection thread serving exactly one request. PATCH-FPCHTTP-1 (ListenWithConfig) is unaffected; upstream already carries it.
The test opened a fresh connection per request, because
TFPHTTPClient.KeepConnection defaults to False. The stall only occurs on a
REUSED connection - Linux starts every socket in quickack mode, so the first
ACK is immediate and Nagle never engages. It therefore passed identically with
or without the fix. Setting KeepConnection := True is what makes it a guard.
Adds a compile-time refusal below FPC 3.3.1. The provider's keep-alive and
TCP_NODELAY code sits behind that version guard, so on 3.2.2 the server closes
after each response and the run dies with 'Error reading data from socket' - a
socket error naming nothing relevant.
Rewrites the header, which asserted the disproved cause ('a fixed ~40 ms
select() poll interval'). The real cause is Nagle interacting with the peer's
delayed ACK, established by wire capture: the body segment leaves 11-14 us
after the ACK, across an ACK delay that itself varies 42.5-43.9 ms.
Verified against 3.3.4 on FPC trunk: 30 requests on one connection, max 1 ms.
Dockerfile.tests-lazarus adds fp-units-web, which this test needs for
fphttpclient. .gitignore covers both FPC test binaries: their Linux builds have
no extension, so the existing *.exe rule does not catch them.
Standalone FPC program plus runner, covering the FIX-WS-NONBLOCK path. Horse's own suite is DUnitX and Delphi-only, so the FPC + epoll transport has no way to be tested from it; this was written for the review of PR HashLoad#549 and kept after the maintainer reworked that fix rather than merging ours. Verified against the reworked implementation now in 3.3.4 - errno captured once and passed to both predicates, close deferred via FStateLock plus an FActiveOperations refcount - which is code this test was not written for. 5 checks, all passing on FPC 3.2.2. The load-bearing assertion is SILENCE, not liveness. epoll sets O_NONBLOCK on accepted sockets, so recv returns -1/EAGAIN whenever no frame has arrived yet; the original defect read that as a disconnect, broke the upgrader's read loop on its first iteration and let the HTTP pipeline resume, writing a stray 'HTTP/1.1 200 OK' onto a socket already handed to WebSocket. An earlier draft asserted only that the connection was still open, and passed against the broken code. The runner refuses to build a tree lacking WS_SOCKET_READ_TICK_MS, so it cannot report a pass on a checkout that predates the fix. Exit code is the number of failed checks, so it gates directly.
…s image Our version of Dockerfile.tests-lazarus deleted upstream's WebSocketEpollRegression build-and-run step and replaced it with a build of FPCHttpKeepaliveTest, so the Lazarus test image silently stopped covering the non-blocking WebSocket transport. The replacement comment also restated the disproved cause of the keep-alive stall - a fixed ~40 ms select() poll interval - superseded by the Nagle/delayed-ACK finding in horse#562. Reverted to upstream's version outright rather than patched, because the rest of our change is no longer wanted either: fp-units-web was added only for that keep-alive build step, and nothing else in the image uses fphttpclient. This file is now byte-identical to upstream, which is one less thing to reconcile at the next sync. The keep-alive test is deliberately NOT re-added here. It now refuses to build below FPC 3.3.1, where the provider's keep-alive and TCP_NODELAY code is not compiled at all, and this image installs Ubuntu 22.04's fpc 3.2.2 - so adding it would fail the image build. It runs standalone against trunk instead.
# Conflicts: # .gitignore # tests/fpc-websocket/WebSocketEagainRegression.lpr # tests/fpc-websocket/run-ws-eagain-test.sh
The HTTP.sys provider access-violates when sending a response whose headers were set only via Res.RawWebResponse.SetCustomHeader with no Res.AddHeader call - exactly what Horse.CORS does. SendResponse's Delphi branch enumerates the shadow CustomHeaders dict (for LPair in LHeadersList), which is nil in that case, while the raw header count is > 0 so the block is still entered: EAccessViolation (Read of 0x00000004). The FPC branch above is already safe (for I := 0 to LHeaderCount - 1, which is 0 when the dict is nil). Fix: a one-line nil guard before the for..in. Regression test (samples/tests): new route /raw/setheader-only (SetCustomHeader only, no AddHeader - keeps the shadow dict nil) plus client Section 37. Negative control on the HTTP.sys test server: unfixed -> AV + Section 37 fails (101 passed / 2 failed) fixed -> 104 passed / 0 failed (sections 1-36 unaffected) The route is provider-agnostic and also passes on Indy / Epoll / IOCP. Files: src/Horse.Provider.HttpSys.pas samples/tests/Horse.Indy.TestRoutes.pas samples/tests/HorseNetHttpTestClient.dpr
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
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.
Summary
The HTTP.sys provider access-violates when sending a response whose headers were set only through
Res.RawWebResponse.SetCustomHeaderwith noRes.AddHeadercall — which is exactly whatHorse.CORSdoes. Every CORS response (and any middleware that writes headers via the raw adapter) crashed the server.Root cause
THttpSysRawResponse.SendResponse, Delphi branch (src/Horse.Provider.HttpSys.pas, ~line 1521):When headers arrive only via
SetCustomHeader, the shadowCustomHeadersdict is never allocated (nil), yet the block is entered because the raw header count is> 0. Enumerating a nilTDictionarydereferences nil →EAccessViolation (Read of address 00000004). The FPC branch directly above is already safe because it loopsfor I := 0 to LHeaderCount - 1, andLHeaderCountis 0 when the dict is nil.Fix
A one-line nil guard before the
for..in:Reproduction & regression test
Added Section 37 to the shared test suite (
samples/tests/):/raw/setheader-onlyinHorse.Indy.TestRoutes.pas— sets a header only viaRawWebResponse.SetCustomHeader, noAddHeader(keeps the shadow dict nil);Section 37inHorseNetHttpTestClient.dprasserting200+ body + theX-Raw-Onlyheader.Verified against the HTTP.sys test server (
HorseHttpSysTestServer, port 9010):EAccessViolation (Read of 0x00000004); Section 37 fails (status 0 / timeout); 101 passed / 2 failedSections 1–36 are unaffected either way, which isolates the crash to this exact path. The route is provider-agnostic — it also passes on the Indy / Epoll / IOCP test servers, confirming the guard is HttpSys-specific.
Files
src/Horse.Provider.HttpSys.pas— the nil guard.samples/tests/Horse.Indy.TestRoutes.pas—/raw/setheader-onlyroute.samples/tests/HorseNetHttpTestClient.dpr— Section 37.