From fab5f58afb5d9e692b71b191dbfa364c978dc4a4 Mon Sep 17 00:00:00 2001 From: nlattice <280584158+nlattice@users.noreply.github.com> Date: Wed, 27 May 2026 08:07:37 +0000 Subject: [PATCH] fix(wish/cpp): clean up session data for more error cases --- wish/cpp/.gitignore | 1 + wish/cpp/CMakeLists.txt | 4 ++ wish/cpp/README.md | 58 ++++++++++++++++++++++++++ wish/cpp/src/h2_server.cc | 76 +++++++++++++++++++++++----------- wish/cpp/src/h2_server.h | 4 ++ wish/cpp/src/h2_server_test.cc | 72 ++++++++++++++++++++++++++++++++ wish/cpp/src/h2_tls_server.cc | 60 ++++++++++++++++----------- wish/cpp/src/h2_tls_server.h | 3 ++ 8 files changed, 230 insertions(+), 48 deletions(-) create mode 100644 wish/cpp/src/h2_server_test.cc diff --git a/wish/cpp/.gitignore b/wish/cpp/.gitignore index c5e9848..7615099 100644 --- a/wish/cpp/.gitignore +++ b/wish/cpp/.gitignore @@ -1,5 +1,6 @@ _deps build +build_asan/ *.log *.a diff --git a/wish/cpp/CMakeLists.txt b/wish/cpp/CMakeLists.txt index 12eabb5..064a72b 100644 --- a/wish/cpp/CMakeLists.txt +++ b/wish/cpp/CMakeLists.txt @@ -245,4 +245,8 @@ if(WISH_BUILD_TESTS) add_executable(handshake_test src/handshake_test.cc) target_link_libraries(handshake_test http1_handshake gtest_main gtest event) add_test(NAME handshake_test COMMAND handshake_test) + + add_executable(h2_server_test src/h2_server_test.cc) + target_link_libraries(h2_server_test web_stream gtest_main gtest event) + add_test(NAME h2_server_test COMMAND h2_server_test) endif() diff --git a/wish/cpp/README.md b/wish/cpp/README.md index 00cb9d4..80bbcbb 100644 --- a/wish/cpp/README.md +++ b/wish/cpp/README.md @@ -2,8 +2,66 @@ This directory contains a C++ implementation of WiSH protocol together with a thin TLS stack enabling mTLS. +## Building + +To build the C++ targets (libraries, examples, benchmarks, and tests) using Clang and Ninja: + +```bash +# Configure and build using Ninja and Clang +cmake \ + -B build \ + -G Ninja \ + -DCMAKE_C_COMPILER=clang \ + -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_BUILD_TYPE=Debug && \ + cmake --build build -- -j$(nproc) +``` + +To run unit tests after a standard build: + +```bash +ctest --test-dir build --output-on-failure +``` + # Development Conventions ## Coding style We basically follow [Google C++ style guide](https://google.github.io/styleguide/cppguide.html). + +## Testing and Memory Leak Detection (ASan / LSan) + +To verify correctness and detect memory leaks (such as orphaned session objects on failed connection handshakes), run unit tests with **AddressSanitizer (ASan)** and **LeakSanitizer (LSan)** enabled. + +### 1. Building with ASan / LSan + +Pass `-fsanitize=address` to compiler and linker flags during CMake configuration: + +```bash +# Configure build with ASan / LSan instrumentation +cmake -B build_asan -S . \ + -G Ninja \ + -DCMAKE_C_COMPILER=clang \ + -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_BUILD_TYPE=Debug \ + -DCMAKE_CXX_FLAGS="-fsanitize=address -g" \ + -DCMAKE_C_FLAGS="-fsanitize=address -g" \ + -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" + +# Build test targets +cmake --build build_asan +``` + +### 2. Running Tests with Leak Monitoring + +Run `ctest` against the ASan-instrumented build directory: + +```bash +# Run all tests with leak detection enabled +ctest --test-dir build_asan --output-on-failure + +# Run a specific test (e.g., h2_server_test) +ctest --test-dir build_asan -R h2_server_test --output-on-failure +``` + +If a memory leak or invalid memory access occurs during test execution, LeakSanitizer/AddressSanitizer will print a detailed stack trace and cause the test to fail. diff --git a/wish/cpp/src/h2_server.cc b/wish/cpp/src/h2_server.cc index 296f371..ab2ace7 100644 --- a/wish/cpp/src/h2_server.cc +++ b/wish/cpp/src/h2_server.cc @@ -68,6 +68,20 @@ bool H2Server::Init() { return true; } +int H2Server::GetPort() const { + if (listener_) { + evutil_socket_t fd = evconnlistener_get_fd(listener_); + if (fd >= 0) { + struct sockaddr_in sin; + socklen_t len = sizeof(sin); + if (getsockname(fd, reinterpret_cast(&sin), &len) == 0) { + return ntohs(sin.sin_port); + } + } + } + return port_; +} + void H2Server::SetOnStream(StreamCallback cb) { on_stream_ = cb; } int H2Server::Run() { @@ -144,9 +158,7 @@ void H2Server::AcceptConnCb(evconnlistener* listener, VLOG(1) << "H2Server: nghttp2_submit_settings() failed: " << nghttp2_strerror(submit_settings_rv); - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -159,9 +171,7 @@ void H2Server::AcceptConnCb(evconnlistener* listener, VLOG(1) << "H2Server: nghttp2_session_set_local_window_size() failed: " << nghttp2_strerror(set_local_window_size_rv); - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -171,9 +181,7 @@ void H2Server::AcceptConnCb(evconnlistener* listener, VLOG(1) << "H2Server: nghttp2_session_send() failed: " << nghttp2_strerror(send_rv); - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -189,9 +197,7 @@ void H2Server::AcceptConnCb(evconnlistener* listener, if (enable_rv != 0) { VLOG(1) << "H2Server: bufferevent_enable() failed"; - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -214,6 +220,31 @@ void H2Server::AcceptErrorCb(evconnlistener* listener, // ---- libevent bufferevent callbacks ---- +void H2Server::HandleSessionError(Session* sess) { + if (!sess) { + return; + } + for (auto& [sid, info] : sess->incoming_streams) { + if (info.web_stream) { + info.web_stream->OnError(); + delete info.web_stream; + } + } + sess->incoming_streams.clear(); + + if (sess->h2session) { + nghttp2_session_del(sess->h2session); + sess->h2session = nullptr; + } + + if (sess->bev) { + bufferevent_free(sess->bev); + sess->bev = nullptr; + } + + delete sess; +} + void H2Server::ReadCallback(bufferevent* bev, void* ctx) { Session* sess = static_cast(ctx); @@ -232,7 +263,7 @@ void H2Server::ReadCallback(bufferevent* bev, void* ctx) { VLOG(1) << "H2Server: nghttp2_session_mem_recv() failed: " << nghttp2_strerror(static_cast(readlen)); - bufferevent_free(bev); + HandleSessionError(sess); return; } @@ -240,7 +271,7 @@ void H2Server::ReadCallback(bufferevent* bev, void* ctx) { if (drain_rv != 0) { VLOG(3) << "H2Server: evbuffer_drain() failed"; - bufferevent_free(bev); + HandleSessionError(sess); return; } @@ -249,12 +280,18 @@ void H2Server::ReadCallback(bufferevent* bev, void* ctx) { if (session_send_rv < 0) { VLOG(1) << "H2Server: nghttp2_session_send() failed: " << nghttp2_strerror(session_send_rv); + + HandleSessionError(sess); + + return; } } void H2Server::EventCallback(bufferevent* bev, short what, // NOLINT(runtime/int) void* ctx) { + (void)bev; + Session* sess = static_cast(ctx); if (what & BEV_EVENT_ERROR) { @@ -262,16 +299,7 @@ void H2Server::EventCallback(bufferevent* bev, } if (what & (BEV_EVENT_EOF | BEV_EVENT_ERROR)) { - for (auto& [sid, info] : sess->incoming_streams) { - if (info.web_stream) { - info.web_stream->OnError(); - delete info.web_stream; - } - } - - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); } } diff --git a/wish/cpp/src/h2_server.h b/wish/cpp/src/h2_server.h index 1ae508b..3cae974 100644 --- a/wish/cpp/src/h2_server.h +++ b/wish/cpp/src/h2_server.h @@ -41,6 +41,7 @@ class H2Server { ~H2Server(); bool Init(); + int GetPort() const; void SetOnStream(StreamCallback cb); int Run(); @@ -115,6 +116,9 @@ class H2Server { nghttp2_data_source*, void*); + // Helper: tear down session resources on error or closure. + static void HandleSessionError(Session* sess); + // Helper: initialise an nghttp2 server session for a new connection. static nghttp2_session* CreateH2Session(Session* sess); diff --git a/wish/cpp/src/h2_server_test.cc b/wish/cpp/src/h2_server_test.cc new file mode 100644 index 0000000..2068d3c --- /dev/null +++ b/wish/cpp/src/h2_server_test.cc @@ -0,0 +1,72 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "h2_server.h" + +#include +#include +#include +#include +#include +#include +#include + +#include + +class H2ServerTest : public ::testing::Test { + protected: + void SetUp() override { + base_ = event_base_new(); + ASSERT_NE(base_, nullptr); + } + + void TearDown() override { + if (base_) { + event_base_free(base_); + } + } + + event_base* base_ = nullptr; +}; + +TEST_F(H2ServerTest, InvalidConnectionPrefaceCleanlyDeallocatesSession) { + // Bind to port 0 (ephemeral port) + H2Server server(base_, 0); + ASSERT_TRUE(server.Init()); + + // Connect client socket to local loopback port + int client_fd = socket(AF_INET, SOCK_STREAM, 0); + ASSERT_GE(client_fd, 0); + + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_port = htons(server.GetPort()); + inet_pton(AF_INET, "127.0.0.1", &addr.sin_addr); + + ASSERT_EQ(connect(client_fd, reinterpret_cast(&addr), sizeof(addr)), 0); + + // Dispatch event loop briefly to accept the connection. + event_base_loop(base_, EVLOOP_NONBLOCK); + + // Send malformed connection preface instead of valid HTTP/2 magic. + const char kInvalidPreface[] = "INVALID_CLIENT_MAGIC_BYTES_PAYLOAD"; + ssize_t bytes_written = send(client_fd, kInvalidPreface, sizeof(kInvalidPreface) - 1, 0); + ASSERT_EQ(bytes_written, sizeof(kInvalidPreface) - 1); + + // Process the read callback which handles the preface failure. + event_base_loop(base_, EVLOOP_NONBLOCK); + + close(client_fd); + event_base_loop(base_, EVLOOP_NONBLOCK); +} diff --git a/wish/cpp/src/h2_tls_server.cc b/wish/cpp/src/h2_tls_server.cc index 938fa89..26b3aea 100644 --- a/wish/cpp/src/h2_tls_server.cc +++ b/wish/cpp/src/h2_tls_server.cc @@ -187,9 +187,7 @@ void H2TlsServer::AcceptConnCb(evconnlistener* listener, VLOG(1) << "H2TlsServer: nghttp2_submit_settings() failed: " << nghttp2_strerror(submit_settings_rv); - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -201,9 +199,7 @@ void H2TlsServer::AcceptConnCb(evconnlistener* listener, VLOG(1) << "H2TlsServer: nghttp2_session_set_local_window_size() failed: " << nghttp2_strerror(set_local_window_size_rv); - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -213,9 +209,7 @@ void H2TlsServer::AcceptConnCb(evconnlistener* listener, VLOG(1) << "H2TlsServer: nghttp2_session_send() failed: " << nghttp2_strerror(send_rv); - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -231,9 +225,7 @@ void H2TlsServer::AcceptConnCb(evconnlistener* listener, if (enable_rv != 0) { VLOG(1) << "H2TlsServer: bufferevent_enable() failed"; - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); return; } @@ -256,6 +248,31 @@ void H2TlsServer::AcceptErrorCb(evconnlistener* listener, // ---- libevent bufferevent callbacks ---- +void H2TlsServer::HandleSessionError(Session* sess) { + if (!sess) { + return; + } + for (auto& [sid, info] : sess->incoming_streams) { + if (info.web_stream) { + info.web_stream->OnError(); + delete info.web_stream; + } + } + sess->incoming_streams.clear(); + + if (sess->h2session) { + nghttp2_session_del(sess->h2session); + sess->h2session = nullptr; + } + + if (sess->bev) { + bufferevent_free(sess->bev); + sess->bev = nullptr; + } + + delete sess; +} + void H2TlsServer::ReadCallback(bufferevent* bev, void* ctx) { Session* sess = static_cast(ctx); @@ -274,7 +291,7 @@ void H2TlsServer::ReadCallback(bufferevent* bev, void* ctx) { VLOG(1) << "H2TlsServer: nghttp2_session_mem_recv() failed: " << nghttp2_strerror(static_cast(recv_len)); - bufferevent_free(bev); + HandleSessionError(sess); return; } @@ -282,7 +299,7 @@ void H2TlsServer::ReadCallback(bufferevent* bev, void* ctx) { if (drain_rv != 0) { VLOG(3) << "H2TlsServer: evbuffer_drain() failed"; - bufferevent_free(bev); + HandleSessionError(sess); return; } @@ -290,12 +307,16 @@ void H2TlsServer::ReadCallback(bufferevent* bev, void* ctx) { if (session_send_rv < 0) { VLOG(1) << "H2TlsServer: nghttp2_session_send() failed: " << nghttp2_strerror(session_send_rv); + + HandleSessionError(sess); + return; } } void H2TlsServer::EventCallback(bufferevent* bev, short what, // NOLINT(runtime/int) void* ctx) { + (void)bev; Session* sess = static_cast(ctx); if (what & BEV_EVENT_ERROR) { @@ -303,16 +324,7 @@ void H2TlsServer::EventCallback(bufferevent* bev, } if (what & (BEV_EVENT_EOF | BEV_EVENT_ERROR)) { - for (auto& [sid, info] : sess->incoming_streams) { - if (info.web_stream) { - info.web_stream->OnError(); - delete info.web_stream; - } - } - - nghttp2_session_del(sess->h2session); - bufferevent_free(bev); - delete sess; + HandleSessionError(sess); } } diff --git a/wish/cpp/src/h2_tls_server.h b/wish/cpp/src/h2_tls_server.h index a537bbe..fb6f654 100644 --- a/wish/cpp/src/h2_tls_server.h +++ b/wish/cpp/src/h2_tls_server.h @@ -115,6 +115,9 @@ class H2TlsServer { nghttp2_data_source*, void*); + // Helper: tear down session resources on error or closure. + static void HandleSessionError(Session* sess); + static nghttp2_session* CreateH2Session(Session* sess); event_base* base_;