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..ce7ac089 100644 --- a/httpfs/test/CMakeLists.txt +++ b/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 () 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