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

ast: Out of bounds memory access crash in AST Vititor #8437

Closed
valeneiko opened this issue Jan 11, 2025 · 4 comments · Fixed by #8455
Closed

ast: Out of bounds memory access crash in AST Vititor #8437

valeneiko opened this issue Jan 11, 2025 · 4 comments · Fixed by #8455
Labels
C-bug Category - Bug

Comments

@valeneiko
Copy link
Contributor

Summary

After adding some unrelated code to a function in my project oxc::ast::Visit::visit_program started crashing with out-of-bounds memory error:

error: process didn't exit successfully: `target\debug\test-runner.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

Stack Trace

Attaching a debugger to the program I got the following stack trace:

test-runner.exe!oxc_span::atom::Atom::as_str() Line 34 (c:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_span-0.44.0\src\atom.rs:34)
test-runner.exe!oxc_span::atom::impl$17::eq<ref$<str$>>(oxc_span::atom::Atom * self, ref$<str$> * other) Line 159 (c:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_span-0.44.0\src\atom.rs:159)
test-runner.exe!oxc_ast::ast::js::Directive::is_use_strict() Line 787 (c:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_ast-0.44.0\src\ast_impl\js.rs:787)
test-runner.exe!core::ops::function::FnMut::call_mut<bool (*)(ref$<oxc_ast::ast::js::Directive>),tuple$<ref$<oxc_ast::ast::js::Directive>>>(bool(*)(oxc_ast::ast::js::Directive *) *, oxc_ast::ast::js::Directive *) Line 166 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:166)
test-runner.exe!core::slice::iter::impl$182::any<oxc_ast::ast::js::Directive,bool (*)(ref$<oxc_ast::ast::js::Directive>)>(core::slice::iter::Iter<oxc_ast::ast::js::Directive> * self, bool(*)(oxc_ast::ast::js::Directive *) f) Line 285 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\slice\iter\macros.rs:285)
test-runner.exe!oxc_ast::ast::js::Program::has_use_strict_directive() Line 20 (c:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_ast-0.44.0\src\ast_impl\js.rs:20)
test-runner.exe!oxc_ast::generated::visit::walk::walk_program<test_runner::TypeVisitorImpl<core::iter::adapters::flatten::FlatMap<core::slice::iter::Iter<alloc::vec::Vec<test_runner::Assertion,alloc::alloc::Global>>,core::slice::iter::Iter<test_runner::Assertion>,test_runner::impl$0::run::closure_env$1>>>(test_runner::TypeVisitorImpl<core::iter::adapters::flatten::FlatMap<core::slice::iter::Iter<alloc::vec::Vec<test_runner::Assertion,alloc::alloc::Global>>,core::slice::iter::Iter<test_runner::Assertion>,test_runner::impl$0::run::closure_env$1>> * visitor, oxc_ast::ast::js::Program * it) Line 1347 (c:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_ast-0.44.0\src\generated\visit.rs:1347)
test-runner.exe!oxc_ast::generated::visit::Visit::visit_program<test_runner::TypeVisitorImpl<core::iter::adapters::flatten::FlatMap<core::slice::iter::Iter<alloc::vec::Vec<test_runner::Assertion,alloc::alloc::Global>>,core::slice::iter::Iter<test_runner::Assertion>,test_runner::impl$0::run::closure_env$1>>>(test_runner::TypeVisitorImpl<core::iter::adapters::flatten::FlatMap<core::slice::iter::Iter<alloc::vec::Vec<test_runner::Assertion,alloc::alloc::Global>>,core::slice::iter::Iter<test_runner::Assertion>,test_runner::impl$0::run::closure_env$1>> * self, oxc_ast::ast::js::Program * it) Line 52 (c:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_ast-0.44.0\src\generated\visit.rs:52)
test-runner.exe!test_runner::TypeVisitor::run() Line 59 (c:\Users\valja\source\rust\rustify\crates\test_runner\src\main.rs:59)
test-runner.exe!test_runner::main() Line 15 (c:\Users\valja\source\rust\rustify\crates\test_runner\src\main.rs:15)
test-runner.exe!core::ops::function::FnOnce::call_once<void (*)(),tuple$<>>(void(*)()) Line 250 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:250)
test-runner.exe!std::sys::backtrace::__rust_begin_short_backtrace<void (*)(),tuple$<>>(void(*)() f) Line 160 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\backtrace.rs:160)
test-runner.exe!std::rt::lang_start::closure$0<tuple$<>>(std::rt::lang_start::closure_env$0<tuple$<>> *) Line 195 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:195)
[Inline Frame] test-runner.exe!std::rt::lang_start_internal::closure$1() Line 174 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:174)
[Inline Frame] test-runner.exe!std::panicking::try::do_call() Line 557 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:557)
[Inline Frame] test-runner.exe!std::panicking::try() Line 520 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:520)
[Inline Frame] test-runner.exe!std::panic::catch_unwind() Line 358 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:358)
test-runner.exe!std::rt::lang_start_internal() Line 174 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:174)
test-runner.exe!std::rt::lang_start<tuple$<>>(void(*)() main, __int64 argc, unsigned char * * argv, unsigned char sigpipe) Line 194 (c:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:194)
test-runner.exe!main() (Unknown Source:0)

It seems that self.directives is NULL here:

/// Returns `true` if this program has a `"use strict"` directive.
pub fn has_use_strict_directive(&self) -> bool {
self.directives.iter().any(Directive::is_use_strict)
}

{__0={buf={ptr=NonNull(0x0000000000000008: 0x0000000000000008 {span={start=??? end=??? } expression={span={start=??? end=??? } value={__0=??? } ...} ...}) ...} ...} }

Code to reproduce (click to expand)

use oxc::{
  allocator::{self, Allocator, IntoIn},
  ast::{AstKind, Visit},
  semantic::Semantic,
};

/// # Panics
pub fn main() {
  let alloc = oxc::allocator::Allocator::default();
  let semantic = parse_file("test.ts", &alloc);
  let visitor = TypeVisitor {
    semantic: &semantic,
    assertions: vec![vec![Assertion { expr: "Foo", expected_type: "Foo" }]],
  };
  visitor.run();
}

fn parse_file<'a>(path: &'_ str, alloc: &'a Allocator) -> Semantic<'a> {
  let source_text: allocator::String = "class Foo {}".to_string().into_in(alloc);
  let source_text = source_text.into_bump_str();
  let source_type = oxc::span::SourceType::from_path(path).unwrap();
  let parser = oxc::parser::Parser::new(alloc, source_text, source_type);

  let parse_result = parser.parse();

  let program = parse_result.program;
  let builder = oxc::semantic::SemanticBuilder::new().with_check_syntax_error(true);
  let semantic_result = builder.build(&program);

  semantic_result.semantic
}

pub struct Assertion<'a> {
  pub expr: &'a str,
  pub expected_type: &'a str,
}

pub struct TypeVisitor<'a> {
  pub semantic: &'a Semantic<'a>,
  pub assertions: Vec<Vec<Assertion<'a>>>,
}

impl TypeVisitor<'_> {
  /// # Panics
  pub fn run(&self) {
    // !!! Removing this statement fixes the crash
    let _ = self.assertions.iter().flat_map(|x| x.iter().map(|x| x.expr)).collect::<String>();

    let source_text = self.semantic.source_text();
    let assertions = self.assertions.iter().flat_map(|x| x.iter());
    let mut visitor = TypeVisitorImpl { _source_text: source_text, _assertions: assertions };
    let AstKind::Program(program) =
      self.semantic.nodes().root_node().expect("root node to exist").kind()
    else {
      panic!("Expected root AST node to be Program");
    };
    visitor.visit_program(program);
  }
}

struct TypeVisitorImpl<'a, T: Iterator<Item = &'a Assertion<'a>>> {
  _source_text: &'a str,
  _assertions: T,
}

impl<'a, T: Iterator<Item = &'a Assertion<'a>>> Visit<'a> for TypeVisitorImpl<'a, T> {}

System Details

  • oxc: 0.44.0
  • rustc 1.84.0 (9fc6b4312 2025-01-07)
    binary: rustc
    commit-hash: 9fc6b43126469e3858e2fe86cafb4f0fd5068869
    commit-date: 2025-01-07
    host: x86_64-pc-windows-msvc
    release: 1.84.0
    LLVM version: 19.1.5
    
  • os: Windows 11
@valeneiko valeneiko added the C-bug Category - Bug label Jan 11, 2025
@Boshen
Copy link
Member

Boshen commented Jan 12, 2025

I cannot reproduce this, but I suspect this:

let source_text: allocator::String = "class Foo {}".to_string().into_in(alloc);
let source_text = source_text.into_bump_str();

The crash hints access to non-existing source text.

Can you try let source_text = "class Foo {}" without storing it in the allocator? Or let source_text = alloc.alloc_str("class Foo {}") if you want it inside the allocator.

@valeneiko
Copy link
Contributor Author

With a static &str outside allocator result is exactly the same.
Using alloc_str result changes - it now crashes on unsafe check due to actual null pointer.

The crash this time is at Vec::deref for self.directives, but this time they are actually NULL:

{__0={buf={ptr=NonNull(0x0000000000000000: 0x0000000000000000 <NULL>) cap=0 alloc=0x0000000000000000 <NULL> } ...} }

With the following stack trace (this time also printed on exit):

unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869\library/std\src\panicking.rs:665
   1: core::panicking::panic_nounwind_fmt
             at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869\library/core\src\intrinsics\mod.rs:3535
   2: core::panicking::panic_nounwind
             at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869\library/core\src\panicking.rs:223
   3: core::slice::raw::from_raw_parts::precondition_check
             at C:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ub_checks.rs:69
   4: core::slice::raw::from_raw_parts<oxc_ast::ast::js::Directive>
             at C:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ub_checks.rs:76
   5: allocator_api2::stable::vec::impl$7::deref
             at C:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\allocator-api2-0.2.21\src\stable\vec\mod.rs:2557
   6: oxc_ast::ast::js::Program::has_use_strict_directive
             at C:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_ast-0.44.0\src\ast_impl\js.rs:19
   7: oxc_ast::generated::visit::walk::walk_program<test_runner::TypeVisitorImpl<core::iter::adapters::flatten::FlatMap<core::slice::iter::Iter<alloc::vec::Vec<test_runner::Assertion,alloc::alloc::Global> >,core::slice::iter::Iter<test_runner::Assertion>,test_ru
             at C:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_ast-0.44.0\src\generated\visit.rs:1347
   8: oxc_ast::generated::visit::Visit::visit_program<test_runner::TypeVisitorImpl<core::iter::adapters::flatten::FlatMap<core::slice::iter::Iter<alloc::vec::Vec<test_runner::Assertion,alloc::alloc::Global> >,core::slice::iter::Iter<test_runner::Assertion>,test_
             at C:\Users\valja\.cargo\registry\src\index.crates.io-6f17d22bba15001f\oxc_ast-0.44.0\src\generated\visit.rs:51
   9: test_runner::TypeVisitor::run
             at .\crates\test_runner\src\main.rs:56
  10: test_runner::main
             at .\crates\test_runner\src\main.rs:15
  11: core::ops::function::FnOnce::call_once<void (*)(),tuple$<> >
             at C:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:250
  12: core::hint::black_box
             at C:\Users\valja\.rustup\toolchains\1.84.0-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\hint.rs:389

I have also tried running the same code on Linux:

  • original code and static &str fail on the same unsafe precondition check
  • alloc_str seems to work

And on M1 Mac all 3 failed with the unsafe precondition check.

Just pushed the code to a repo. Maybe this will help to reproduce it: https://github.com/valeneiko/oxc-crash

@valeneiko
Copy link
Contributor Author

I have found the cause for the crashes: Program node is allocated on the stack, and semantic holds a reference to it. The unrelated line that made it crash performs a bunch of stack allocations ovewriting the program node, and making data invalid. So when we try to access it later - program crashes.

Normally Rust does not allow returning something that references a local var, but for some reason it does here. I am not sure why. But also I was expecting program node to get allocated in the allocator and not on the stack.

Replacing that unrelated iteration with a simple stack allocation also reproduces the issue:

-    let _ = self.assertions.iter().flat_map(|x| x.iter().map(|x| x.expr)).collect::<String>();
+    let arr1 = [0usize; 1];
+    let mut arr = [0usize; 4096];
+    arr[4095] = arr1[0];

@overlookmotel
Copy link
Contributor

I believe the root of the problem is the lifetime extension in this code:

fn alloc<T>(&self, t: &T) -> &'a T {
// SAFETY:
// This should be safe as long as `src` is an reference from the allocator.
// But honestly, I'm not really sure if this is safe.
unsafe { std::mem::transmute(t) }
}

Please see #8461 for discussion of the broader problem, and why it causes this issue.

On this specific issue, I think we can solve it fairly simply:

Make SemanticBuilder::build take a &'a Program<'a>. This makes the lifetime extension in Visit::alloc valid, and removes the unsoundness.

No doubt this will cause compilation errors elsewhere in codebase from the borrow-checker. We'll need to fix them. The borrow-checker is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category - Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants