From 7b7fcae098e3f7171b560abc24e36c25774cc00e Mon Sep 17 00:00:00 2001 From: Arun Sharma Date: Mon, 31 Aug 2026 20:42:38 -0700 Subject: [PATCH 1/2] fix(httpfs): initialize HTTP client on size-cache hit to avoid SIGSEGV on repeat opens Fixes https://github.com/LadybugDB/ladybug/issues/880. 05c63dd introduced a process-wide cache of remote file sizes so that openFile() skips the HEAD round trip on repeat opens of the same URL. However, the fast-path early return in HTTPFileInfo::initMetadata() was placed before initializeClient(), leaving httpClient null on every open after the first. The first ranged GET on such a handle then dereferenced the null httplib client and crashed with SIGSEGV. Because every scan opens the same remote URL at least twice (bind + execution), *all* remote reads on 0.20.x crashed - local reads were unaffected, and 404/403 URLs raised a clean error because they fail during the first open's HEAD. initializeClient() only constructs an httplib::Client (no network I/O), so moving it above the size-cache lookup restores the invariant that an initialized HTTPFileInfo always has a client while keeping the HEAD deduplication. Also add defensive null-client guards to head/get/post/ put requests so no code path can dereference an uninitialized client. Adds a gtest regression test (httpfs_httpfs_test) that serves a file over a local HTTP server, opens it twice, and reads from the second handle - it segfaults without the fix and passes with it - plus a test asserting repeat opens still issue only one HEAD. Reported-by: jfrench9 --- httpfs/src/httpfs.cpp | 23 ++++- httpfs/test/CMakeLists.txt | 3 + httpfs/test/httpfs_test.cpp | 170 ++++++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 1 deletion(-) create mode 100644 httpfs/test/httpfs_test.cpp diff --git a/httpfs/src/httpfs.cpp b/httpfs/src/httpfs.cpp index 7cf3e0db..5e3107f6 100644 --- a/httpfs/src/httpfs.cpp +++ b/httpfs/src/httpfs.cpp @@ -211,6 +211,12 @@ HTTPFileInfo::HTTPFileInfo(std::string path, FileSystem* fileSystem, int flags, httpConfig{context}, cachedFileInfo{nullptr} {} void HTTPFileInfo::initMetadata() { + // The HTTP client must be initialized on every open, including the fast + // path below: a size-cache hit skips the HEAD request, but any subsequent + // read still dereferences httpClient. Leaving it null here caused a + // SIGSEGV on the first ranged GET of every repeat open of the same URL + // (https://github.com/LadybugDB/ladybug/issues/880). + initializeClient(); // Remote files are immutable during a query; reuse the file size learned by // an earlier openFile() for the same URL instead of paying a fresh HEAD + // redirect round trip per open. This is what turns N opens of the same @@ -225,7 +231,6 @@ void HTTPFileInfo::initMetadata() { } auto hfs = fileSystem->ptrCast(); - initializeClient(); auto res = hfs->headRequest(this->ptrCast(), path, {}); std::string rangeLength; if (res->code != 200) { @@ -932,6 +937,11 @@ std::unique_ptr HTTPFileSystem::headRequest(FileInfo* fileInfo, return runBrowserRequestWithRetry("HEAD", url, std::move(headerMap)); #else auto httpFileInfo = dynamic_cast_checked(fileInfo); + // Defensive: guard against any open path that did not go through + // initMetadata() (e.g. a future fast-path open) leaving the client null. + if (!httpFileInfo->httpClient) { + httpFileInfo->initializeClient(); + } auto parsedURL = parseUrl(url); auto host = parsedURL.first; auto hostPath = parsedURL.second; @@ -969,6 +979,11 @@ std::unique_ptr HTTPFileSystem::getRangeRequest(FileInfo* fileInfo return response; #else auto httpFileInfo = dynamic_cast_checked(fileInfo); + // Defensive: guard against any open path that did not go through + // initMetadata() (e.g. a future fast-path open) leaving the client null. + if (!httpFileInfo->httpClient) { + httpFileInfo->initializeClient(); + } auto parsedURL = parseUrl(url); auto host = parsedURL.first; auto hostPath = parsedURL.second; @@ -1049,6 +1064,9 @@ std::unique_ptr HTTPFileSystem::postRequest(common::FileInfo* file return response; #else auto httpFileInfo = dynamic_cast_checked(fileInfo); + if (!httpFileInfo->httpClient) { + httpFileInfo->initializeClient(); + } auto hostPath = parseUrl(url).second; auto headers = getHTTPHeaders(headerMap); uint64_t outputBufferPos = 0; @@ -1091,6 +1109,9 @@ std::unique_ptr HTTPFileSystem::putRequest(common::FileInfo* fileI inputBufferLen); #else auto httpFileInfo = dynamic_cast_checked(fileInfo); + if (!httpFileInfo->httpClient) { + httpFileInfo->initializeClient(); + } auto hostPath = parseUrl(url).second; auto headers = getHTTPHeaders(headerMap); std::function request([&]() { diff --git a/httpfs/test/CMakeLists.txt b/httpfs/test/CMakeLists.txt index 9987a3e3..2a966ac2 100644 --- a/httpfs/test/CMakeLists.txt +++ b/httpfs/test/CMakeLists.txt @@ -1,4 +1,7 @@ if (${BUILD_EXTENSION_TESTS}) add_lbug_test(httpfs_xetfs_test xetfs_test.cpp) target_link_libraries(httpfs_xetfs_test PRIVATE httpfs_extension_source ${OPENSSL_LIBRARIES}) + + add_lbug_test(httpfs_httpfs_test httpfs_test.cpp) + target_link_libraries(httpfs_httpfs_test PRIVATE httpfs_extension_source ${OPENSSL_LIBRARIES}) endif () diff --git a/httpfs/test/httpfs_test.cpp b/httpfs/test/httpfs_test.cpp new file mode 100644 index 00000000..44f9aa2f --- /dev/null +++ b/httpfs/test/httpfs_test.cpp @@ -0,0 +1,170 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "common/file_system/file_system.h" +#include "common/file_system/virtual_file_system.h" +#include "gtest/gtest.h" +#include "httpfs.h" +#include "httpfs_extension.h" +#include "httplib.h" +#include "main/client_context.h" +#include "main/connection.h" +#include "main/database.h" + +using namespace lbug; +using namespace lbug::common; +using namespace lbug::httpfs_extension; + +namespace { + +// Serves a directory over HTTP with range-request support so the httpfs code +// paths can be exercised without network access. +class LocalHttpServer { +public: + explicit LocalHttpServer(const std::string& mountDir) { + if (!server_.set_mount_point("/", mountDir.c_str())) { + throw std::runtime_error("Failed to mount directory " + mountDir); + } + server_.set_pre_routing_handler([this](const httplib::Request& req, httplib::Response&) { + if (req.method == "HEAD") { + ++headCount_; + } + return httplib::Server::HandlerResponse::Unhandled; + }); + for (int port = 18123; port < 18153; ++port) { + if (server_.bind_to_port("127.0.0.1", port)) { + port_ = port; + break; + } + } + if (port_ == 0) { + throw std::runtime_error("Failed to bind local http server"); + } + thread_ = std::thread([this]() { server_.listen_after_bind(); }); + while (!server_.is_running()) { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + } + + ~LocalHttpServer() { + server_.stop(); + if (thread_.joinable()) { + thread_.join(); + } + } + + std::string urlFor(const std::string& fileName) const { + return "http://127.0.0.1:" + std::to_string(port_) + "/" + fileName; + } + + int headCount() const { return headCount_; } + +private: + httplib::Server server_; + std::thread thread_; + std::atomic headCount_{0}; + int port_ = 0; +}; + +std::string makeTestContent(size_t size) { + std::string content; + content.reserve(size); + for (size_t i = 0; i < size; ++i) { + content.push_back(static_cast(i % 251)); + } + return content; +} + +class HttpFileSystemTest : public ::testing::Test { +public: + void SetUp() override { + dir_ = ::testing::TempDir() + "httpfs_test"; + std::filesystem::create_directories(dir_); + content_ = makeTestContent(100000); + server_ = std::make_unique(dir_); + + database_ = std::make_unique(":memory:"); + connection_ = std::make_unique(database_.get()); + context_ = connection_->getClientContext(); + HttpfsExtension::load(context_); + vfs_ = VirtualFileSystem::GetUnsafe(*context_); + } + + // Writes the fixture content under a unique name and points url_ at it. + // Each test uses its own file name because the remote file size cache is + // process-global and would otherwise leak across tests. + void setupDataFile(const std::string& fileName) { + std::ofstream out(dir_ + "/" + fileName, std::ios::binary | std::ios::trunc); + out.write(content_.data(), static_cast(content_.size())); + url_ = server_->urlFor(fileName); + } + + void TearDown() override { + connection_.reset(); + database_.reset(); + server_.reset(); + } + + std::unique_ptr openUrl() { + return vfs_->openFile(url_, FileOpenFlags(FileFlags::READ_ONLY), context_); + } + +protected: + std::string content_; + std::string dir_; + std::string url_; + std::unique_ptr server_; + std::unique_ptr database_; + std::unique_ptr connection_; + main::ClientContext* context_ = nullptr; + VirtualFileSystem* vfs_ = nullptr; +}; + +// Regression test for https://github.com/LadybugDB/ladybug/issues/880: +// the second open of the same URL used to hit the process-wide size cache and +// return before initializing the HTTP client, so the first ranged read on that +// handle dereferenced a null httplib client and crashed. +TEST_F(HttpFileSystemTest, ReadAfterRepeatOpenOfSizeCachedUrl) { + setupDataFile("data_repeat_open.bin"); + // First open: performs the HEAD request and populates the size cache. + auto firstHandle = openUrl(); + ASSERT_EQ(content_.size(), firstHandle->getFileSize()); + + std::vector buf(content_.size()); + firstHandle->readFromFile(buf.data(), buf.size(), 0); + EXPECT_EQ(0, std::memcmp(buf.data(), content_.data(), content_.size())); + + // Second open of the same URL: size-cache hit, HEAD is skipped. + auto secondHandle = openUrl(); + ASSERT_EQ(content_.size(), secondHandle->getFileSize()); + + // This read used to SIGSEGV because httpClient was left null. + std::fill(buf.begin(), buf.end(), '\0'); + secondHandle->readFromFile(buf.data(), buf.size(), 0); + EXPECT_EQ(0, std::memcmp(buf.data(), content_.data(), content_.size())); + + // And a read at a non-zero offset for good measure. + secondHandle->readFromFile(buf.data(), 4096, content_.size() - 4096); + EXPECT_EQ(0, std::memcmp(buf.data(), content_.data() + content_.size() - 4096, 4096)); +} + +TEST_F(HttpFileSystemTest, RepeatOpenDoesNotIssueSecondHead) { + setupDataFile("data_head_count.bin"); + // The size cache exists to avoid a HEAD per open; make sure the client + // initialization fix did not reintroduce the HEAD round trip. + auto firstHandle = openUrl(); + ASSERT_EQ(content_.size(), firstHandle->getFileSize()); + auto secondHandle = openUrl(); + ASSERT_EQ(content_.size(), secondHandle->getFileSize()); + EXPECT_EQ(1, server_->headCount()); +} + +} // namespace From 7bb9d0482f8412cdbfd02afdaedaa5eb19d620f5 Mon Sep 17 00:00:00 2001 From: Arun Sharma Date: Mon, 31 Aug 2026 22:49:24 -0700 Subject: [PATCH 2/2] fix(httpfs): compile httpfs tests with CPPHTTPLIB_OPENSSL_SUPPORT CI (manylinux_2_28, Release) crashed with SIGSEGV in httplib::ClientImpl::create_and_connect_socket in the new httpfs tests. Cause: httpfs_extension_source is compiled with CPPHTTPLIB_OPENSSL_SUPPORT while the test TUs included httplib.h without it. httplib types change layout depending on that flag (e.g. httplib::Socket gains an `SSL*` member), so mixing both variants in one binary is an ODR violation; the linker merges the inline member definitions and the wrong copy reads the wrong offsets. Locally the linker happened to pick a compatible copy, which is why this was not caught before pushing. Compile all httpfs test targets with the same flag so every TU in the binary sees the same httplib layout. --- httpfs/test/CMakeLists.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/httpfs/test/CMakeLists.txt b/httpfs/test/CMakeLists.txt index 2a966ac2..ce7ac089 100644 --- a/httpfs/test/CMakeLists.txt +++ b/httpfs/test/CMakeLists.txt @@ -4,4 +4,13 @@ if (${BUILD_EXTENSION_TESTS}) add_lbug_test(httpfs_httpfs_test httpfs_test.cpp) target_link_libraries(httpfs_httpfs_test PRIVATE httpfs_extension_source ${OPENSSL_LIBRARIES}) + + # httpfs_extension_source is compiled with CPPHTTPLIB_OPENSSL_SUPPORT, and + # the test TUs include httplib.h as well. Mixing the two variants in one + # binary is an ODR violation (e.g. httplib::Socket gains an `SSL*` member + # when the flag is defined), which manifests as SIGSEGV inside httplib + # client code depending on the linker's choice of inline definitions. + # Keep the flags consistent across all TUs that include httplib.h. + target_compile_definitions(httpfs_xetfs_test PRIVATE CPPHTTPLIB_OPENSSL_SUPPORT) + target_compile_definitions(httpfs_httpfs_test PRIVATE CPPHTTPLIB_OPENSSL_SUPPORT) endif ()