fix(httpfs): initialize HTTP client on size-cache hit to avoid SIGSEGV on repeat opens

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<HTTPFileSystem>();
-    initializeClient();
     auto res = hfs->headRequest(this->ptrCast<HTTPFileInfo>(), path, {});
     std::string rangeLength;
     if (res->code != 200) {
@@ -932,6 +937,11 @@ std::unique_ptr<HTTPResponse> HTTPFileSystem::headRequest(FileInfo* fileInfo,
     return runBrowserRequestWithRetry("HEAD", url, std::move(headerMap));
 #else
     auto httpFileInfo = dynamic_cast_checked<HTTPFileInfo*>(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<HTTPResponse> HTTPFileSystem::getRangeRequest(FileInfo* fileInfo
     return response;
 #else
     auto httpFileInfo = dynamic_cast_checked<HTTPFileInfo*>(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<HTTPResponse> HTTPFileSystem::postRequest(common::FileInfo* file
     return response;
 #else
     auto httpFileInfo = dynamic_cast_checked<HTTPFileInfo*>(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<HTTPResponse> HTTPFileSystem::putRequest(common::FileInfo* fileI
         inputBufferLen);
 #else
     auto httpFileInfo = dynamic_cast_checked<HTTPFileInfo*>(fileInfo);
+    if (!httpFileInfo->httpClient) {
+        httpFileInfo->initializeClient();
+    }
     auto hostPath = parseUrl(url).second;
     auto headers = getHTTPHeaders(headerMap);
     std::function<httplib::Result(void)> request([&]() {

httpfs/test/CMakeLists.txt

@@ -1,4 +1,16 @@
 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})
+
+    # 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 ()

httpfs/test/httpfs_test.cpp

@@ -0,0 +1,170 @@
+#include <algorithm>
+#include <atomic>
+#include <chrono>
+#include <cstring>
+#include <filesystem>
+#include <fstream>
+#include <memory>
+#include <string>
+#include <thread>
+#include <vector>
+
+#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<int> 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<char>(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<LocalHttpServer>(dir_);
+
+        database_ = std::make_unique<main::Database>(":memory:");
+        connection_ = std::make_unique<main::Connection>(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<std::streamsize>(content_.size()));
+        url_ = server_->urlFor(fileName);
+    }
+
+    void TearDown() override {
+        connection_.reset();
+        database_.reset();
+        server_.reset();
+    }
+
+    std::unique_ptr<FileInfo> openUrl() {
+        return vfs_->openFile(url_, FileOpenFlags(FileFlags::READ_ONLY), context_);
+    }
+
+protected:
+    std::string content_;
+    std::string dir_;
+    std::string url_;
+    std::unique_ptr<LocalHttpServer> server_;
+    std::unique_ptr<main::Database> database_;
+    std::unique_ptr<main::Connection> 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<char> 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