Просмотр исходного кода

Fix mbedTLS is_peer_closed() destroying the first response byte

Mbed TLS has no SSL_peek() equivalent, so is_peer_closed() (called after
every SSL request write) probed liveness with a real 1-byte
mbedtls_ssl_read() and discarded whatever it read. If the response had
already arrived by the time the probe ran — plausible under CI load or
plain OS scheduling — the probe silently ate the first byte of the
status line, corrupting the response and surfacing as a fast
"Failed to read connection" failure.

This was the root cause of the long-standing MbedTLS-only CI flakiness
(ServerTest cases failing intermittently on Ubuntu and macOS), previously
worked around by reducing gtest shard parallelism. Fix: push the probed
byte back into MbedTlsSession and have tls::read()/pending() account for
it, so no data is lost.

Also fix a second, unrelated flake: ProxyTunnelTest.
OriginReturning407InsideTunnelDoesNotLeakProxyDigest used "localhost" for
its client while the test's proxy harness only listens on 127.0.0.1;
under dual-stack resolution this could race with another test's server
on ::1 using the same ephemeral port. Pin the test to 127.0.0.1.

With the root cause fixed, restore the mbedTLS CI jobs (ubuntu,
ubuntu-26.04, macOS) to the default shard count instead of the
previously reduced SHARDS=1/2 mitigation.
yhirose 2 недель назад
Родитель
Сommit
2fa0417754
3 измененных файлов с 50 добавлено и 23 удалено
  1. 3 12
      .github/workflows/test.yaml
  2. 41 9
      httplib.h
  3. 6 2
      test/test.cc

+ 3 - 12
.github/workflows/test.yaml

@@ -104,10 +104,7 @@ jobs:
           LSAN_OPTIONS: suppressions=lsan_suppressions.txt
       - name: build and run tests (Mbed TLS)
         if: matrix.tls_backend == 'mbedtls'
-        # Run mbedTLS shards with reduced parallelism — under ASAN+mbedTLS the
-        # default 4 shards overload CI runners enough that timing-sensitive
-        # ServerTest cases flake on first-request keep-alive reuse.
-        run: cd test && make test_split_mbedtls && SHARDS=2 make test_mbedtls_parallel
+        run: cd test && make test_split_mbedtls && make test_mbedtls_parallel
       - name: build and run tests (wolfSSL)
         if: matrix.tls_backend == 'wolfssl'
         run: cd test && make test_split_wolfssl && make test_wolfssl_parallel
@@ -142,10 +139,7 @@ jobs:
       - name: install Mbed TLS
         run: sudo apt-get install -y libmbedtls-dev
       - name: build and run tests (Mbed TLS)
-        # Run mbedTLS shards with reduced parallelism — under ASAN+mbedTLS the
-        # default 4 shards overload CI runners enough that timing-sensitive
-        # ServerTest cases flake on first-request keep-alive reuse.
-        run: cd test && make test_split_mbedtls && SHARDS=2 make test_mbedtls_parallel
+        run: cd test && make test_split_mbedtls && make test_mbedtls_parallel
 
   # BoringSSL is Google's fork of OpenSSL. It has no API stability guarantee
   # and is not packaged by distros, so we build it from source. cpp-httplib
@@ -410,10 +404,7 @@ jobs:
           LSAN_OPTIONS: suppressions=lsan_suppressions.txt
       - name: build and run tests (Mbed TLS)
         if: matrix.tls_backend == 'mbedtls'
-        # macOS runners under ASAN+mbedTLS still flake at SHARDS=2 (rapid
-        # bind/connect on the fixture's fixed port races on the slower
-        # macos-latest runner). Serialize fully here; ubuntu stays at 2.
-        run: cd test && make test_split_mbedtls && SHARDS=1 make test_mbedtls_parallel
+        run: cd test && make test_split_mbedtls && make test_mbedtls_parallel
       - name: build and run tests (wolfSSL)
         if: matrix.tls_backend == 'wolfssl'
         run: cd test && make test_split_wolfssl && make test_wolfssl_parallel

+ 41 - 9
httplib.h

@@ -18038,6 +18038,13 @@ struct MbedTlsSession {
   std::string hostname;     // For client: set via set_sni
   std::string sni_hostname; // For server: received from client via SNI callback
 
+  // Mbed TLS has no SSL_peek() equivalent, so is_peer_closed() must probe with
+  // a real 1-byte mbedtls_ssl_read(). If that probe lands on application data
+  // (e.g. a response that arrived while this side was still in its post-write
+  // check), the byte is pushed back here and served by the next read().
+  unsigned char peeked_byte = 0;
+  bool has_peeked_byte = false;
+
   MbedTlsSession() { mbedtls_ssl_init(&ssl); }
 
   ~MbedTlsSession() { mbedtls_ssl_free(&ssl); }
@@ -18743,6 +18750,23 @@ inline ssize_t read(session_t session, void *buf, size_t len, TlsError &err) {
   }
 
   auto msession = static_cast<impl::MbedTlsSession *>(session);
+
+  // Serve a byte consumed by the is_peer_closed() probe before reading more.
+  if (msession->has_peeked_byte) {
+    if (len == 0) { return 0; }
+    auto p = static_cast<unsigned char *>(buf);
+    p[0] = msession->peeked_byte;
+    msession->has_peeked_byte = false;
+    size_t n = 1;
+    // Top up with any already-decrypted bytes without risking a block.
+    if (len > 1 && mbedtls_ssl_get_bytes_avail(&msession->ssl) > 0) {
+      int extra = mbedtls_ssl_read(&msession->ssl, p + 1, len - 1);
+      if (extra > 0) { n += static_cast<size_t>(extra); }
+    }
+    err.code = ErrorCode::Success;
+    return static_cast<ssize_t>(n);
+  }
+
   int ret;
   do {
     ret = mbedtls_ssl_read(&msession->ssl, static_cast<unsigned char *>(buf),
@@ -18802,7 +18826,8 @@ inline int pending(const_session_t session) {
   if (!session) { return 0; }
   auto msession =
       static_cast<impl::MbedTlsSession *>(const_cast<void *>(session));
-  return static_cast<int>(mbedtls_ssl_get_bytes_avail(&msession->ssl));
+  return static_cast<int>(mbedtls_ssl_get_bytes_avail(&msession->ssl)) +
+         (msession->has_peeked_byte ? 1 : 0);
 }
 
 inline void shutdown(session_t session, bool graceful) {
@@ -18828,19 +18853,21 @@ inline bool is_peer_closed(session_t session, socket_t sock) {
   if (!session || sock == INVALID_SOCKET) { return true; }
   auto msession = static_cast<impl::MbedTlsSession *>(session);
 
-  // Check if there's already decrypted data available in the TLS buffer
-  // If so, the connection is definitely alive
-  if (mbedtls_ssl_get_bytes_avail(&msession->ssl) > 0) { return false; }
+  // Check if there's already decrypted or pushed-back data available.
+  // If so, the connection is definitely alive.
+  if (msession->has_peeked_byte ||
+      mbedtls_ssl_get_bytes_avail(&msession->ssl) > 0) {
+    return false;
+  }
 
   // Set socket to non-blocking to avoid blocking on read
   detail::set_nonblocking(sock, true);
   auto cleanup =
       detail::scope_exit([&]() { detail::set_nonblocking(sock, false); });
 
-  // Try a 1-byte read to check connection status
-  // Note: This will consume the byte if data is available, but for the
-  // purpose of checking if peer is closed, this should be acceptable
-  // since we're only called when we expect the connection might be closing
+  // Probe with a 1-byte read (Mbed TLS has no peek API). If the probe lands
+  // on application data — e.g. a response that already arrived — push the
+  // byte back so the next read() delivers it instead of losing it.
   unsigned char buf;
   int ret;
   do {
@@ -18848,7 +18875,12 @@ inline bool is_peer_closed(session_t session, socket_t sock) {
   } while (impl::mbedtls_is_session_ticket(ret));
 
   // If we got data or WANT_READ (would block), connection is alive
-  if (ret > 0 || ret == MBEDTLS_ERR_SSL_WANT_READ) { return false; }
+  if (ret > 0) {
+    msession->peeked_byte = buf;
+    msession->has_peeked_byte = true;
+    return false;
+  }
+  if (ret == MBEDTLS_ERR_SSL_WANT_READ) { return false; }
 
   // If we get a peer close notify or a connection reset, the peer is closed
   return ret == MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY ||

+ 6 - 2
test/test.cc

@@ -20893,9 +20893,13 @@ TEST(ProxyTunnelTest, OriginReturning407InsideTunnelDoesNotLeakProxyDigest) {
   proxy_tunnel_test::ScopedConnectProxy proxy(origin.port());
   ASSERT_NE(0, proxy.port());
 
-  SSLClient cli(HOST, origin.port());
+  // Pin to 127.0.0.1: the proxy listens on 127.0.0.1 only, while "localhost"
+  // may resolve to ::1 first. A concurrent test (e.g. another gtest shard)
+  // holding ::1 with the same ephemeral port number would hijack the CONNECT
+  // and answer with its own status, making this test flaky.
+  SSLClient cli("127.0.0.1", origin.port());
   cli.enable_server_certificate_verification(false);
-  cli.set_proxy(HOST, proxy.port());
+  cli.set_proxy("127.0.0.1", proxy.port());
   cli.set_proxy_digest_auth("proxy-user", "proxy-pass");
 
   auto res = cli.Get("/x");