Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Boa wrongly assumes that allocating never fails and writes to a null pointer #1847

Closed
y21 opened this issue Feb 19, 2022 · 1 comment
Closed
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution memory PRs and Issues related to the memory management or memory footprint.
Milestone

Comments

@y21
Copy link
Contributor

y21 commented Feb 19, 2022

Boa assumes that std::alloc::alloc never fails (returning a null pointer) even though it can.

boa/boa/src/string.rs

Lines 219 to 223 in 29cd909

let inner = unsafe {
let inner = alloc(layout).cast::<Self>();
// Write the first part, the Inner.
inner.write(Self {

Quoting the docs:

unsafe fn alloc(&self, layout: Layout) -> *mut u8
Allocate memory as described by the given layout.
Returns a pointer to newly-allocated memory, or null to indicate allocation failure.

If the allocation fails, then there will be a write on a null pointer on L223, which is considered UB:

  • A null pointer is never valid, not even for accesses of size zero.

This also leads to violating the NonNull::new_unchecked invariant:

ptr must be non-null.

unsafe { NonNull::new_unchecked(inner) }

To Reproduce

use boa::JsString;

fn main() {
    // choose a string length that's large enough to cause an allocation error when concatenated later but
    // not too large so the initial allocation still succeeds
    let string = "a".repeat(1000 * 1000 * 1000);
    let ve = vec![&string as &str; 1000];

    // this is almost certainly going to cause an allocation error unless you've got lots of RAM
    // if it does, then this will write to a null pointer 💥
    let _ = JsString::concat_array(&ve);
}

Alternatively, one can override the global allocator and return null for that specific allocation: link.
Implementing GlobalAlloc requires unsafe due to the nature of allocators being unsafe, but I'm fairly certain it's totally valid (albeit weird) for a crate to expose a safe API for an allocator like that.
Running the code under valgrind sums it up:

==22393== Process terminating with default action of signal 11 (SIGSEGV)
==22393==  Access not within mapped region at address 0x0
==22393==    at 0x110477: boa::string::JsString::concat_array
==22393==    by 0x11019F: boatest::main

[1]    22393 segmentation fault (core dumped)  valgrind ./target/release/boatest

Expected behavior
Handling null pointers, e.g. assert!(!inner.is_null()). Or better yet, calling std::alloc:handle_alloc_error as suggested in the docs

Build environment

  • OS: Ubuntu
  • Version: 20.04
  • Target triple: x86_64-unknown-linux-gnu (installed)
  • Rustc version: rustc 1.60.0-nightly (75d9a0ae2 2022-02-16)
@y21 y21 added the bug Something isn't working label Feb 19, 2022
@Razican
Copy link
Member

Razican commented Feb 19, 2022

Maybe this is addressed by #1659

@Razican Razican added builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution memory PRs and Issues related to the memory management or memory footprint. labels Feb 19, 2022
@bors bors bot closed this as completed in fabbf15 Feb 20, 2022
Razican pushed a commit that referenced this issue Feb 21, 2022
Fixes #1847 by wrapping the `std::alloc::alloc()` call in `try_alloc()`, which checks that the returned pointer is non-null and handles allocation errors that way. It will now abort the process instead of executing UB in the error path
@Razican Razican added this to the v0.14.0 milestone Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution memory PRs and Issues related to the memory management or memory footprint.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants