Fix: Undefined Behavior in BoxedBytes::from_concat on Zero-Length Input#2395
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
chain/core/src/types/boxed_bytes.rs:85
alloc(layout)may return a null pointer on OOM. Using it unconditionally withadd/copy_nonoverlappingis UB in a safe function. Check for null and callalloc::alloc::handle_alloc_error(layout)(or otherwise handle OOM) before using the pointer.
let layout = Layout::from_size_align(result_len, core::mem::align_of::<u8>()).unwrap();
let result_ptr = alloc(layout);
let mut current_index = 0usize;
andrei-marinica
changed the title
|
Contract comparison - from 4267d66 to eba6564
|
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
BoxedBytes::from_concatcalledalloc(Layout { size: 0, align: 1 })when allinput slices were empty or the slice list was empty. Calling the global allocator
with a zero-sized layout violates Rust's allocator contract and produces undefined
behavior.
Root Cause
The unsafe allocation path had no guard for
result_len == 0. The fix adds anearly return via
BoxedBytes::empty()before entering theunsafeblock.BoxedBytes::empty()boxes a[u8; 0]ZST, which the stdlib handles entirely atcompile time using
NonNull::dangling()— the allocator is never invoked.Impact by Allocator
MultiversX smart contracts can be compiled with one of four allocators, each with
different behavior when
alloc(size=0)is called:alloc(size=0)behaviordeallocbehaviorAllocationForbidden(default)signal_error("memory allocation forbidden")— hard contract abortfrom_concatunconditionally terminates the contract with a user-visible error, regardless of whether the business logic would otherwise succeed.WeeAllocLeakingAllocator0 as *mut u8(null pointer) when no pages have been requested yetBox. Technically UB;as_ptr()returns null. In practice harmless because dealloc is a no-op and no data is read through the zero-length slice, but the pointer value is wrong.StaticAllocator64KheadSeverity assessment
WeeAlloc: High. Silent heap corruption within a live contract execution.An attacker who can supply empty byte arguments to an endpoint that calls
from_concatcan trigger the free-list corruption and cause subsequentallocations to alias live contract state.
AllocationForbidden: Medium. Reliable denial of service against thecontract endpoint. No state corruption, but the contract aborts instead of
executing.
LeakingAllocator/StaticAllocator64K: Low. Technically UB; nopractical corruption observed due to no-op dealloc.
Fix
Added before the
unsafeblock infrom_concat.BoxedBytes::empty()nevercalls the allocator under any of the four contract allocators.
Tests Added
test_concat_single_empty_slice— regression: single&b""previously hit the UB pathtest_concat_single_nonempty_slice— single non-empty slice round-trips correctlytest_concat_leading_empty_slices— empty slices before a non-empty slicetest_concat_trailing_empty_slices— empty slices after a non-empty slicetest_concat_result_is_empty_instance— all-empty concat returns a valid, dereferenceable instance