[TRACKING] ext/http embedded server correctness and safety audit follow-ups

[thc1006]

While fixing #4281 and #4282 I read through the rest of ext/http and found more than fits in one PR. This is a tracker rather than a single bug: each item below has its own root cause, its own platform surface and its own fix, and I am opening a child issue and a small PR per item as I get to them rather than filing everything at once.

Every item is a code-inspection finding against main at 9d38caf. The affected code path and the conditions that reach it are described per item. Runtime reproduction and regression-test status belong in the child issues, not here. Please push back on anything you consider a deliberate limitation of a test server rather than a defect, and I will drop it.

One further potentially security-sensitive finding was reported privately under the project security policy and is intentionally out of scope here.

Memory safety

• [BUG] HttpServer accesses a Connection after handleConnectionClosed() erases it #4288: processRequest() uses a Connection after handleConnectionClosed() erases it. Fixed by [TEST] Fix use-after-free when an HTTP handler closes the connection #4289.
• [BUG] Out-of-bounds write in SocketAddr(char const *) on Windows #4290: out-of-bounds write in SocketAddr(char const *) on Windows, where the copy loop uses a byte count as an element count. Fixed by [TEST] Make SocketAddr string parsing safe and reject malformed addresses #4292.
• [BUG] SocketAddr(char const *) reads uninitialized bytes when parsing host:port #4291: SocketAddr(char const *) reads uninitialized bytes because the host buffer is terminated at a fixed index. Fixed by [TEST] Make SocketAddr string parsing safe and reject malformed addresses #4292.

Process safety

• [BUG] Embedded HTTP server writes can raise SIGPIPE and terminate the process #4293: writes can raise SIGPIPE and terminate the host process. PR [BUG] Stop embedded HTTP server writes from raising SIGPIPE #4294 is up. Worth landing before the fatal send error item below, since otherwise the process can die before that handling runs.

Nonblocking write handling

• A partial write can stall a response on Windows. sendMore() performs a single send() per readiness event. Winsock permits a nonblocking send() to succeed having transferred only part of the buffer, and documents that after the first FD_WRITE an application should keep sending until a send fails with WSAEWOULDBLOCK, because that failure is what arms the next FD_WRITE. Re-registering the same interest does not help: socket_tools.h#L582 only calls WSAEventSelect when the flag set actually changes, so a repeat addSocket(Writable | Closed) is a no-op. A short write on a response larger than the send buffer can therefore park in SendingHeaders or SendingBody. Draining until the socket says stop also makes the would-block branch reachable by a test, which it is not today under level-triggered readiness. Credit for this one goes to a reviewer of [BUG] Fix transient would-block handling in the ext/http embedded server #4283.
• A fatal send() error never closes the connection. http_server.h#L344-L347 returns true on any non would-block error without closing the socket, removing the reactor registration or changing state. On Linux EPOLLOUT is registered without EPOLLET, so the writable callback can be re-entered continuously; on Windows there may simply be no further event. [BUG] Fix transient would-block handling in the ext/http embedded server #4283 fixes the would-block cases and deliberately leaves this one.
• EINTR is not classified as retryable. An interrupted recv is treated as a peer close and an interrupted send leaves the connection stalled. socket_tools.h#L770 already handles EINTR for the poll loop, so there is a precedent to follow.
• Response length is narrowed to int. http_server.h#L342 casts conn.sendBuffer.size() to int before passing it to a wrapper that takes unsigned. The response body comes from the handler, so m_maxRequestContentSize does not bound it. A body around 4 GiB can narrow to a length of 0, which makes send() return 0 and the buffer stop advancing.

Reactor (all platforms)

• Interest-registration backend calls are unchecked. WSACreateEvent, epoll_ctl(EPOLL_CTL_ADD), WSAEventSelect and epoll_ctl(EPOLL_CTL_MOD), and the kevent add calls, ignore their return values, while the teardown kevent calls do check theirs. A failed registration updates the software interest flags but leaves the kernel out of sync silently, so a socket can be waited on for the wrong events or never woken. Credit for this one goes to a reviewer of [BUG] Stop embedded HTTP server writes from raising SIGPIPE #4294.
• A combined readable-and-close event drops queued data. The Linux dispatch calls onSocketReadable() and then onSocketClosed() as separate ifs in one iteration, and onSocketReadable() does a single recv() of one buffer. For EPOLLIN | EPOLLHUP (the peer sent more than one buffer and then closed) the reactor reads one buffer and closes; the close removes the socket, so there is no later readable event to drain the rest. The Windows path has the same shape at FD_READ then FD_CLOSE. Both epoll and Winsock allow readable data to remain when a hangup or close is reported. Credit for this one goes to a reviewer of [BUG] Fix transient would-block handling in the ext/http embedded server #4283.

macOS reactor

• Interest flags are ignored and EV_EOF is never processed. socket_tools.h#L567-L570 registers both EVFILT_READ and EVFILT_WRITE regardless of the requested flags, and #L627 carries a TODO saying flag updates are unsupported, so a socket registered only for reading is woken continually by writability and then does nothing. Separately, in the event loop the EVFILT_READ and EVFILT_WRITE branches each end in continue while the EV_EOF / EV_ERROR handling sits after them. Since kqueue only reports EV_EOF as a flag on one of those two filters, that close path, and the LOG_ERROR("Reactor: unhandled kevent!") below it, are unreachable.

Request framing

• Ambiguous framing is accepted. Duplicate headers are joined with a comma at http_server.h#L672-L680, and the Content-Length parse at #L440-L451 tolerates trailing characters, so Content-Length: 5 followed by Content-Length: 999 parses as 5. Transfer-Encoding does not appear anywhere in the file, so it is neither honored nor rejected. RFC 9112 requires framing to be unambiguous. The trailing-character tolerance is mine from [CODE HEALTH] Fix clang-tidy bugprone-unchecked-string-to-number-conversion warnings #4216, so tightening it means revisiting that decision, and this is the item most likely to be a deliberate test-server simplification rather than a defect.

Build

• [BUG] file_http_server.h does not compile on Windows #4299: file_http_server.h cannot compile on Windows, because std::replace is applied to a const std::string &. Resolved by Remove file_http_server.h #4306, which removed the header; [BUG] Make file_http_server.h compile on Windows #4300 was closed as superseded.

Minor

• ::tolower and ::toupper receive a plain char at http_server.h#L708, #L724 and #L733, which is undefined for negative values.

Possible follow-up, not a defect

A compile-only target that includes every installed ext header on Linux, macOS and Windows would have caught the build item above, and other installed headers with no in-repo includer are likely in the same position. Happy to propose one if that sounds useful.

---

I do not expect all of these to be accepted, and I would rather you closed some as working-as-intended than have me open PRs for them. If you use formal sub-issues I am happy for these to be attached that way; I do not have the permission to do it myself.

[thc1006]

Three more items surfaced during review of #4283 and #4294. Recording them here so they are tracked rather than folded into those scoped PRs.

• A fatal send() error still does not clean up the connection. After [BUG] Stop embedded HTTP server writes from raising SIGPIPE #4294 suppresses SIGPIPE, a write to a dead peer returns -1/EPIPE instead of killing the process, but sendMore() then only returns true without closing the socket, removing the reactor registration or changing state. This is the same lifecycle gap already listed above; noting the interaction so the SIGPIPE fix is not mistaken for full recovery.
• Linux EPOLLHUP can strand buffered inbound data. When a peer closes with data still unread, the reactor may act on the hangup before draining what is readable, so a final request could be dropped. Needs confirming against the epoll level-triggered semantics.
• Windows FD_CLOSE half-close handling. Winsock delivers FD_CLOSE once and expects the application to keep reading until zero bytes; the current reactor may not drain a half-closed connection the same way epoll/kqueue would.
• Reactor-backend registration errors are ignored. epoll_ctl, kevent and WSAEventSelect return values are not checked in addSocket/removeSocket, so a failed registration is silent.

The first is a known blocker for the fatal-send-error PR; the other three are lower priority and want a maintainer's read on whether they are in scope for this embedded test server at all.

[thc1006]

One more from the #4292 review, a pre-existing one I am deliberately not fixing in that PR because it touches storage layout.

• SocketAddr stores a sockaddr but accesses it as sockaddr_in. socket_tools.h declares sockaddr m_data{} and reads and writes it through reinterpret_cast<sockaddr_in &>(m_data) in several places, which is undefined behaviour under the C++ object model. It works in practice, but it is not guaranteed, and the syscall wrappers also pass sizeof(SocketAddr) as the address length rather than a real socklen_t, which only works because the struct happens to have one member. A proper fix (store sockaddr_in, or a union, plus an explicit length) is a wider change to the whole wrapper, so it belongs in its own issue rather than riding on the memory-safety PR.

[thc1006]

More pre-existing robustness gaps in FileHttpServer::FileGetSuccess / ServeFile, surfaced during review of #4300. Kept out of that PR (it is a scoped build fix) and recorded here. The path-confinement finding is already being handled privately under the security policy and is intentionally not detailed here.

• tellg() failure is not checked. size = file.tellg() can return pos_type(-1); it is then used for resize() and read() without a check, which can mean a huge allocation or a bad conversion. Guard against the -1 sentinel and an implausibly large size.
• file.read() result is ignored. FileGetSuccess returns true whenever the file merely opened, even if the seek failed, the file was truncated after the size was taken, or the read was short or left the stream in a fail state. Check the stream and gcount() and return false on a short read.
• Non-ASCII filenames on Windows. std::ifstream(std::string) uses a narrow filename interpreted in the ANSI code page on MSVC, so UTF-8 paths may not open. A portability limitation rather than a bug.
• URI query and directory inference. ServeFile does not split the query component from the path and decides "directory" by whether the name contains a ., so a static asset with a query, a dotted directory name, or a differently-cased extension can resolve wrongly.

None of these are introduced by #4300; they want their own small PRs if the audit continues.

[dbarker]

@thc1006 Thank you for the thorough analysis and the detailed write-up to track work on the internal http server files. The depth of the review is genuinely appreciated, and the fixes in #4288, #4290, #4291 and the build issue #4299 have been valuable.

I want to share some more context to help you and others prioritize effort. The http_server.h and socket_tools.h files are for internal use only. They exist to support tests and a single HTTP example. Since they are not part of the public API or intended for production use, our bar for them is that they should be stable and correct enough for testing and the example, but we are not aiming for a production-grade HTTP server.

I propose that once #4327 merges to remove both files from the installed package, we close this ticket and the remaining http_server/socket_tools tickets that would add features or production-grade hardening beyond what is needed for internal use. #4332 tracks moving them out of ext/include directory and completing that move will be helpful, but can also take a lower priority to other work.

Does this sound reasonable?

If you are looking for areas where contributions have the most impact, the pinned [ADMIN] issue lists the highest-priority work including spec compliance, code quality, and performance. These are all areas where help is very much needed.

Thanks again for the care you have put into this.

[thc1006]

Thank you, and yes. When I opened this I asked to be told if any of it was a deliberate limitation of a test server rather than a defect, and said I would drop those. You have told me, so I am dropping them.

I have closed #4283 and #4294, since both are hardening on those two files, and I am not opening the four I had queued for them either, on fatal send errors, EINTR, reactor interest flags and request framing. Both PRs can be reopened if you ever want them, but nothing is waiting on you. This issue is yours to close whenever #4327 lands and I will not add to it.

One thing I will record without arguing for it, since it bears on where the testing bar sits rather than on those PRs: an unhandled SIGPIPE ends the test process rather than failing a call. I have not seen it happen in CI.

#4327 is approved and green whenever you want it. #4346 touches ext/http/client/http_client.h and the exporters rather than the server files, so I read it as outside this, but say if you see it differently. Everything else I have open is in the Elasticsearch exporter.

On where the effort should go instead, thank you for pointing at #3068. I have sixteen merged PRs against #2053, so continuing the clang-tidy work under 2.1 is the easiest thing for me to keep doing and needs nothing from you. If the Prometheus specification gaps under 1.6 are worth more, I would rather do those. In that case I would take one item, say which on the issue first, and check the approach before writing anything.