Skip to content

A socket whose body was not read to the end goes back into the pool, and the next request reads the remainder as its status line #15

Description

@yspbwx2010

HttpClient keeps one socket per host:port and reuses it whenever is_valid() holds (http.cppm:356-358 in send_impl, :637-639 in send_stream). is_valid() tests only that the TLS state exists and the descriptor is open (tls.cppm:100-102, socket.cppm:60-62), nothing about whether bytes are still owed on the socket or whether the peer is still there, so reuse is safe only when the previous exchange consumed its whole body. Several exits from the body readers leave before that point with the socket kept:

  • send_stream, Content-Length branch (:877-885): wait_readable timing out (:878-880) and read() returning 0 or less (:884-885) both break with connectionClose and stopped still false, so the clean-up at :909-912 keeps the socket. This branch is not in 0.2.3, where a non-chunked body was read until the connection closed (0.2.3 http.cppm:738-740), which set connectionClose unconditionally; I have not checked the releases in between.
  • send_stream, chunked branch (:846-848): read_exact failing on chunk data breaks out the same way. The neighbouring invalid-chunk-size path at :832-836 does set connectionClose = true (:834); this one does not. 0.2.3 has the same lines.
  • send_impl, chunked branch: the read_exact break at :550-551 has the same shape. It also still parses the size line with parse_hex (:541), so an empty line from a timeout is taken as the terminal chunk at :542-545 and the socket is pooled as if the body had ended; the comment at :822-830 records that parse_chunk_size_line had reached only download_to_file, and it has not reached this reader either.
  • download_to_file_impl: the read_exact failures at :1166-1171 and :1195-1200 return without sock->close() / pool_.erase(poolKey) and skip the epilogue at :1223-1226, whose connectionClose is false in both branches. The failure paths next to them at :1150-1153 and :1180-1184, and the cancelled() lambda at :1102-1112, all erase.

send_impl's Content-Length case is right: a short body erases the pool entry before returning (:562-565), and its read-until-close branch closes (:567-569, :582-585).

Measured

tinyhttps 0.2.10 from the index (mcpp.lock: mcpplibs.tinyhttps 0.2.10), clang 22.1.8, x86_64-linux-gnu. A scripted TLS server (Python, self-signed certificate; the client runs with verifySsl = false) answers the first request on a connection with HTTP/1.1 418, Content-Length: 100, sends 50 bytes and holds the connection open; when a second request arrives on that same connection it sends the 50 owed bytes followed by a complete HTTP/1.1 200 OK / Content-Length: 2 / ok. The chunked case differs in the first response only: Transfer-Encoding: chunked, a 64 chunk header, 50 bytes sent, and 57 bytes owed (the rest of the chunk, its CRLF and the terminal chunk). The client is one HttpClient with readTimeoutMs = 1000 and keepAlive at its default; request 1 goes through send_stream (POST, callback returns true), request 2 through send (GET). The control differs in one thing only: request 2 goes through a fresh HttpClient.

Request 1 returns statusCode=418 statusText="I'm a teapot" in every case. Where the server holds the connection it returns one read timeout late, 1070 to 1074 ms at readTimeoutMs = 1000, carrying 50 of the declared 100 bytes in the Content-Length cases and an empty body in the chunked one, since the partial chunk is never dispatched. What request 2 then sees depends on what the leftover bytes look like:

leftover bytes request 2 on the same client control (fresh client)
44 x B then 999 X statusCode=999 statusText="XHTTP/1.1 200 OK" body="ok" 200 "OK" "ok"
a truncated SSE tail, data: {"error":"teapot"}\r\n\r\ndata: [DONE]\r\n\r\n\r\n\r\n\r\n statusCode=0 statusText="" body.size()=62 elapsed=1001ms; the body is data: [DONE]\r\n\r\n\r\n\r\n\r\nHTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok, the real response swallowed into the body of a status-0 reply after another full read timeout 200 "OK" "ok" in 68 ms
50 x B, no space, no CRLF statusCode=200 statusText="OK" body="ok", which looks right; the server log shows one TCP connection for the whole run against two for the control, and read_line (:169-191) had prepended the 50 bytes to the real status line, where the parser's first-space split (:457) happened to fall inside the genuine HTTP/1.1 200 OK same output, two TCP connections
chunked: 57 bytes owed, starting with 50 x B then CRLF statusCode=0 statusText="Invalid status line" 200 "OK" "ok"

The first row is the one to look at: 999 and XHTTP/1.1 200 OK can only come from the previous body's bytes being read as this response's status line, and Content-Length: 2 and ok are still found after them.

If the server closes instead of holding (same 418 head, 50 bytes, then close), request 1 returns in about 70 ms through the read() <= 0 exit at :885, and the kept entry costs more than wrong data. The client does not reconnect: the server log shows no second connection and no second request. The write of request 2 goes out without error, read_line at :447 then returns empty, and the pool_.erase(poolKey) in the resulting "No response" path at :449 destroys the pooled TlsSocket, whose destructor sends a TLS close-notify into the broken pipe. With SIGPIPE ignored, request 2 returns statusCode=0 statusText="No response". With it at its default disposition, which is what a program that has not disarmed it has, the process is killed: exit status 141. gdb: Socket::write (socket.cppm:178, a bare ::send(fd_, buf, len, 0)) under bio_send (tls.cppm:48) under mbedtls_ssl_close_notify under TlsSocket::close (tls.cppm:151) under ~TlsSocket under send_impl (http.cppm:449). There is no MSG_NOSIGNAL and no SIGPIPE handling anywhere in the package. That is arguably its own defect, since any pooled peer that goes away between requests reaches the same close-notify; I mention it here because this is where it was measured, and can file it separately if you would rather keep the two apart.

What the fix could be

In send_stream, setting connectionClose = true at the three early exits (:879, :885, :847) is enough: the clean-up at :909-912 already keys off it, and the normal terminations (the remaining > 0 loop condition, and the terminal-chunk break at :841) do not go through those exits, so no success path changes. read() returning 0 at :885 is end of stream, so that exit needs it as much as the timeout does. In send_impl the same at :551, plus parse_chunk_size_line in place of parse_hex at :541. In download_to_file_impl the flag would not help, since :1166-1171 and :1195-1200 return before the epilogue; those two want the direct sock->close(); pool_.erase(poolKey); that :1151-1152 and :1182-1183 already do.

The server script and the client are small (one Python file, one main.cpp, a run.sh). If a test would help, I can turn the Content-Length and chunked cases into a gtest against a local listener in the style of test_download.cpp.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions