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

Questo è un tracker con più elementi; inizia da una issue figlia invece che dal tracker, quindi leggi l’entry point indicato ext/include/opentelemetry/ext/http/server/socket_tools.h o http_server.h. Riproduci la condizione indicata nella issue figlia e usa lo stato del relativo test di regressione per definire quando il lavoro è completato; il tracker in sé non fornisce un unico ambito né un obiettivo di test.

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

• #4288: processRequest() uses a Connection after handleConnectionClosed() erases it. Fixed by #4289.
• #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 #4292.
• #4291: SocketAddr(char const *) reads uninitialized bytes because the host buffer is terminated at a fixed index. Fixed by #4292.

Process safety

• #4293: writes can raise SIGPIPE and terminate the host process. PR #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 #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. #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 #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 #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 #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

• #4299: file_http_server.h cannot compile on Windows, because std::replace is applied to a const std::string &. Resolved by #4306, which removed the header; #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.