fix(arm): Reject unsupported page size in MmapAllocator; fix AbfsFileSystemTest teardown crash - #18618
Open
karthikeyann wants to merge 3 commits into
Open
fix(arm): Reject unsupported page size in MmapAllocator; fix AbfsFileSystemTest teardown crash#18618karthikeyann wants to merge 3 commits into
karthikeyann wants to merge 3 commits into
Conversation
When AzuriteServer's constructor throws (for example, `azurite-blob` isn't on PATH), gtest still runs the fixture's `TearDown()` before reporting the `SetUp()` failure. `TearDown()` dereferenced `azuriteServer_` unconditionally, so instead of a clean test failure the whole `velox_abfs_test` binary crashed with SIGSEGV before gtest's exception handler could report the original error. `azuriteServer_` stays null until `AzuriteServer`'s constructor returns successfully, so guard `TearDown()` with a null check. Test Plan: Reproduced by running `velox_abfs_test` with no `azurite-blob` on `PATH`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`MmapAllocator` tracks memory at `AllocationTraits::kPageSize` (4096-byte) granularity and calls `madvise()` on individual pages, which requires addresses and lengths to line up with the OS's actual page size. NVIDIA's documented recommended default page size for Grace and Grace-Hopper systems is 64KB, not 4KB. On such systems `madvise()` silently fails with `EINVAL` on sub-64KB regions instead of throwing, which corrupts the allocator's internal page-count accounting instead of surfacing a clear error. Add `MmapAllocator::isPageSizeSupported()` and check it in the constructor, so construction now fails immediately with a clear message instead of silently corrupting state later. This mirrors a known, currently unresolved limitation in jemalloc, which DuckDB and ArangoDB both bundle and which also doesn't support pages larger than 4KB. Deferred: making `MmapAllocator` actually work with a 64KB page size means generalizing `AllocationTraits::kPageSize` from a compile-time constant to a runtime-queried value across the allocator's size-class and bitmap accounting, which also changes its minimum allocation granularity. That's a larger design change tracked in #18617. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
karthikeyann
requested review from
PingLiuPing and
majetideepak
as code owners
August 21, 2026 04:18
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
Bot
added
the
CLA Signed
This was referenced Aug 21, 2026
Closed
Selective Build Plan
Affected targets (530)Directly changed (17)
Transitively affected (513)
Fast path • Graph from main@522ee471d75a |
karthikeyann
changed the title
Co-authored-by: karthikeyann <6488848+karthikeyann@users.noreply.github.com>
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.
Summary
Two small, independent hardening fixes found while validating aarch64 support (#18587):
AbfsFileSystemTest::TearDown()crash: ifAzuriteServer's constructor throws (e.g.azurite-blobisn't onPATH),TearDown()unconditionally dereferenced the never-constructedazuriteServer_, crashing the wholevelox_abfs_testbinary with SIGSEGV instead of reporting a clean test failure. Guarded with a null check.MmapAllocatorsilently corrupts state on unsupported page sizes:MmapAllocatorassumes the system page size is 4KB (AllocationTraits::kPageSize) for itsmadvise()calls. On hosts where it isn't — notably NVIDIA Grace/Grace-Hopper, whose documented recommended default is 64KB —madvise()fails withEINVALand the allocator's internal accounting silently drifts out of sync instead of erroring. This addsMmapAllocator::isPageSizeSupported()and checks it in the constructor, so construction now fails immediately with a clear, actionable message.This does not make
MmapAllocatorwork on non-4KB-page systems — it converts silent corruption into a loud, clear error. The real fix (generalizing the page-size assumption) is tracked separately in #18617, along with a follow-up draft PR that makes the affected tests skip cleanly on such hosts rather than fail.Test plan
velox_abfs_testwith noazurite-blobonPATH; confirmed it now reports a clean gtest failure instead of SIGSEGV.getconf PAGESIZE= 65536) thatMmapAllocatorconstruction now throws a clearVELOX_CHECKmessage instead of later producing corruptednumMapped()accounting; unaffected on standard 4KB-page hosts (the check is a no-op there).