open-telemetry / open-telemetry/opentelemetry-cpp

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

Open
#4,287 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

build and test
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 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.