Skip to content

Return an allocation error instead of a null pointer from host allocators - #162

Open
kadircanyildirm-crypto wants to merge 1 commit into
nasa:mainfrom
kadircanyildirm-crypto:fix/allocator-null-contract
Open

Return an allocation error instead of a null pointer from host allocators#162
kadircanyildirm-crypto wants to merge 1 commit into
nasa:mainfrom
kadircanyildirm-crypto:fix/allocator-null-contract

Conversation

@kadircanyildirm-crypto

Copy link
Copy Markdown
Contributor

RustSystemAllocator in spacewasm_util reports a failed allocation as success:

unsafe { Ok(std::alloc::alloc(layout)) }

std::alloc::alloc returns NULL on failure, so the caller gets Ok(null). This allocator backs PageAllocator in spacewasi, spacewasm_std and the coremark bench. PageAllocatorInner::alloc propagates a real Err with ? (src/util/paging.rs:125) but cannot see a null Ok, so Page::new(null, ..) is built and the first page is served from address zero.

On main @ 42781cb, forcing the failure with a page size the system cannot satisfy:

std::alloc::alloc(0x4000000000000000) = 0x0
RustSystemAllocator::alloc            -> Ok(0x0)
PageAllocator::alloc                  -> Ok(0x0)
Vec::new_in                           -> Ok, capacity=4
push ...                              -> SIGSEGV

With the change it reports Err(AllocationFailed) instead. crates/spacewasm_util/tests/page_null.rs covers it.

Three other allocators have the same shape and I made them consistent in the same commit, but to be clear: they are reached only through global_allocator!, where GlobalAllocator already null-checks (src/util/alloc.rs:168). I verified they return Err(AllocationFailed) today, so for those three this is consistency, not a fix.

src/ is untouched.

Testing

Clean checkout of main @ 42781cb:

  • cargo build --workspace --all-targets — ok
  • cargo fmt --all -- --check — clean
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
  • cargo test — all targets pass, in debug, release and with strict-assertions
  • cargo miri test -p spacewasm_util — clean. The new test carries #![cfg(not(miri))], since it relies on the real allocator returning NULL, which Miri does not model
  • the new test against the unpatched allocator — fails, as it should

One caveat: custom_page_sizes_integration is OOM-killed in my container (7.6 GB; memory_max asks for 4 GiB). It dies the same way on 42781cb, so that is the environment, not the patch.


Disclosure per AI_POLICY.md. Tool: Claude Code. Type of assistance: tracing the null through PageAllocator, building the reproduction, writing the change, the test and this description. Scope: crates/spacewasm_util/, crates/fuzzing/ and tests/util/spectest.rs — the tooling (crates/*) and test exceptions; no src/ code is touched. Level of modification: the null-checked shape is taken from the existing implementations beside it rather than invented; every command above is one I ran myself.

…tors

std::alloc::alloc returns null on failure, so wrapping it directly in Ok() hands callers a null pointer while the unsafe Allocator trait documents that Ok results must be valid. AllocError::AllocationFailed already exists for this, and the null-checked shape is already used elsewhere in the same files.
@kadircanyildirm-crypto
kadircanyildirm-crypto force-pushed the fix/allocator-null-contract branch from ac9edc5 to 0da4074 Compare August 11, 2026 23:20
@kadircanyildirm-crypto

Copy link
Copy Markdown
Contributor Author

Rebased onto main after #163. The conflict was additive: the 64 MiB cap stays exactly as written and the null check goes under it.

#163 added FuzzAllocator::allocate in the same file using NonNull::new(..).ok_or(AllocError::AllocationFailed). This makes SystemAllocator::alloc, just above it, consistent with that.

Re-verified on d384e10b: build, fmt, clippy and the tests pass, and the new test still fails against the unpatched allocator.

@github-actions

Copy link
Copy Markdown

CoreMark Benchmark Results

Current Score: 245.539
Baseline Score (main): 233.991
Difference: +11.548 (4.94%)

@github-actions

Copy link
Copy Markdown

Code Coverage Report

Current Coverage: 95.30%
Baseline Coverage (main): 95.30%
Difference: +0.00%

@Kronos3 Kronos3 added the Post-v1.0 Issues and features that will not be addressed until after 1.0 Release label Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Post-v1.0 Issues and features that will not be addressed until after 1.0 Release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants