open-telemetry / open-telemetry/opentelemetry-cpp
[TRACKING] ext/http embedded server correctness and safety audit follow-ups
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 1.4k
- Forks
- 632
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 75
Description
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 aConnectionafterhandleConnectionClosed()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
SIGPIPEand 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 singlesend()per readiness event. Winsock permits a nonblockingsend()to succeed having transferred only part of the buffer, and documents that after the firstFD_WRITEan application should keep sending until a send fails withWSAEWOULDBLOCK, because that failure is what arms the nextFD_WRITE. Re-registering the same interest does not help:socket_tools.h#L582only callsWSAEventSelectwhen the flag set actually changes, so a repeataddSocket(Writable | Closed)is a no-op. A short write on a response larger than the send buffer can therefore park inSendingHeadersorSendingBody. 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-L347returnstrueon any non would-block error without closing the socket, removing the reactor registration or changing state. On LinuxEPOLLOUTis registered withoutEPOLLET, 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. -
EINTRis not classified as retryable. An interruptedrecvis treated as a peer close and an interruptedsendleaves the connection stalled.socket_tools.h#L770already handlesEINTRfor the poll loop, so there is a precedent to follow. - Response length is narrowed to
int.http_server.h#L342castsconn.sendBuffer.size()tointbefore passing it to a wrapper that takesunsigned. The response body comes from the handler, som_maxRequestContentSizedoes not bound it. A body around 4 GiB can narrow to a length of0, which makessend()return0and the buffer stop advancing.
Reactor (all platforms)
- Interest-registration backend calls are unchecked.
WSACreateEvent,epoll_ctl(EPOLL_CTL_ADD),WSAEventSelectandepoll_ctl(EPOLL_CTL_MOD), and thekeventadd calls, ignore their return values, while the teardownkeventcalls 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 thenonSocketClosed()as separateifs in one iteration, andonSocketReadable()does a singlerecv()of one buffer. ForEPOLLIN | 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 atFD_READthenFD_CLOSE. Bothepolland 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_EOFis never processed.socket_tools.h#L567-L570registers bothEVFILT_READandEVFILT_WRITEregardless of the requested flags, and#L627carries 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 theEVFILT_READandEVFILT_WRITEbranches each end incontinuewhile theEV_EOF/EV_ERRORhandling sits after them. Since kqueue only reportsEV_EOFas a flag on one of those two filters, that close path, and theLOG_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 theContent-Lengthparse at#L440-L451tolerates trailing characters, soContent-Length: 5followed byContent-Length: 999parses as 5.Transfer-Encodingdoes 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.hcannot compile on Windows, becausestd::replaceis applied to aconst std::string &. Resolved by #4306, which removed the header; #4300 was closed as superseded.
Minor
-
::tolowerand::toupperreceive a plaincharathttp_server.h#L708,#L724and#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.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
This is a multi-item tracker; start with a child issue rather than the tracker, then read the referenced ext/include/opentelemetry/ext/http/server/socket_tools.h or http_server.h entry point. Reproduce the child issue's stated condition and use its regression-test status to define done; the tracker itself does not provide one scope or test target.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, networking, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100