fix(httpfs): initialize HTTP client on size-cache hit to avoid SIGSEGV on repeat opens - #73
Merged
Merged
Conversation
…V on repeat opens Fixes LadybugDB/ladybug#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
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the crash reported in LadybugDB/ladybug#880.
Problem
On 0.20.x, every remote (httpfs) read over
http(s)://segfaults at bind time on the first ranged GET; 0.19.1 and earlier are clean. Local reads of the same files are fine, and URLs that 404/403 raise a cleanRuntimeErrorinstead of crashing.Backtrace (matches the one in #880):
Root cause
05c63dd ("perf(httpfs): pool HTTP connections and cache remote file sizes") added a process-wide cache of remote file sizes so
openFile()skips the HEAD round trip on repeat opens of the same URL. The fast-path early return inHTTPFileInfo::initMetadata()was placed beforeinitializeClient():{ std::lock_guard<std::mutex> lck{HTTPFileSystem::httpSizeCacheMtx}; auto it = HTTPFileSystem::httpSizeCache.find(path); if (it != HTTPFileSystem::httpSizeCache.end()) { length = it->second; return; // ← skips initializeClient() } } ... initializeClient();httpClientis only assigned ininitializeClient(), so the secondopenFile()of the same URL returns a handle whosehttpClientis null. Every scan opens the same remote URL at least twice (bind + execution), and the first ranged GET on the second handle dereferences the null client — hence the SIGSEGV on all remote reads.This also explains the reporter's observations:
Reproduced locally with a
python -m http.serverfile server andLOAD FROM 'http://127.0.0.1:8123/user.parquet' RETURN count(*): deterministic SIGSEGV (exit 139), with the crash point beingmov (%r13),%rsiwherer13 = 0(the null client).Fix
HTTPFileInfo::initMetadata(): callinitializeClient()above the size-cache lookup. It only constructs anhttplib::Client(no network I/O), so the invariant "an initializedHTTPFileInfoalways has a client" is restored while keeping the HEAD deduplication.headRequest/getRangeRequest/postRequest/putRequestso no code path can dereference an uninitialized client.Testing
New regression test
httpfs_httpfs_test(httpfs/test/httpfs_test.cpp), which serves a fixture file over a local range-capable httplib server and exercises the real VFS path:HttpFileSystemTest.ReadAfterRepeatOpenOfSizeCachedUrl: opens the same URL twice, reads (including at a non-zero offset) from the second handle. Segfaults without the fix; passes with it.HttpFileSystemTest.RepeatOpenDoesNotIssueSecondHead: asserts repeat opens still issue exactly one HEAD (the perf win of 05c63dd is preserved).Verified:
httpfs_xetfs_test(4 tests) passes