Skip to content

fix: correct 32-bit layout of ScriptCompiler::Source - #2053

Open
aidilfbk wants to merge 1 commit into
denoland:mainfrom
aidilfbk:fix/script_compiler_source_32bit
Open

aidilfbk wants to merge 1 commit into
denoland:mainfrom
aidilfbk:fix/script_compiler_source_32bit

Conversation

@aidilfbk

@aidilfbk aidilfbk commented Aug 8, 2026

Copy link
Copy Markdown

Hello,

While building rusty_v8 for 32-bit ARMv7 (armv7-unknown-linux-gnueabi), I might have stumbled into a stack buffer overflow in ScriptCompiler::Source. My best guess is that the Rust struct doesn't match the C++ layout on 32-bit platforms. Happy to be corrected.

The ScriptCompiler::Source Rust struct ends in _compilation_details: [usize; 3], however the C++ v8::ScriptCompiler::CompilationDetails expects sizeof(int64_t) * 3, as binding.cc itself asserts ("CompilationDetails size mismatch").

#[repr(C)]
#[derive(Debug)]
pub struct Source {
_source_string: usize,
_resource_name: usize,
_resource_line_offset: int,
_resource_column_offset: int,
_resource_options: int,
_source_map_url: usize,
_host_defined_options: usize,
_cached_data: usize,
_consume_cache_task: usize,
_compile_hint_callback: usize,
_compile_hint_callback_data: usize,
_compilation_details: [usize; 3],
}

rusty_v8/src/binding.cc

Lines 66 to 68 in 8149304

static_assert(sizeof(v8::ScriptCompiler::CompilationDetails) ==
sizeof(int64_t) * 3,
"CompilationDetails size mismatch");

Thus, when Source::new / new_with_cached_data allocates a MaybeUninit::<Self> and passes it to v8__ScriptCompiler__Source__CONSTRUCT, the C++ function writes past the end of the Rust allocation.

impl Source {
#[inline(always)]
pub fn new(
source_string: Local<String>,
origin: Option<&ScriptOrigin>,
) -> Self {
let mut buf = MaybeUninit::<Self>::uninit();
unsafe {
v8__ScriptCompiler__Source__CONSTRUCT(
&mut buf,
&*source_string,
origin.map_or(std::ptr::null(), |x| x as *const _),
std::ptr::null_mut(),
);
buf.assume_init()
}
}

My fix is to change the _compilation_details field to be [u64; 3] regardless of which platform it's built for. I understand 32-bit isn't a supported/CI target so feel free to WONTFIX this.

Thanks for taking a look!

Fixes an issue where on 32-bit systems, the Source struct only reserves 3 u32 spaces for v8::ScriptCompiler::CompilationDetails (asserted as 3 u64 in binding.cc:67 "CompilationDetails size mismatch". Previously resulted in buffer overflow when Source::new is called as Rust doesn't allocate enough space before passing the memory to v8__ScriptCompiler__Source__CONSTRUCT for initialisation
@CLAassistant

CLAassistant commented Aug 8, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@bartlomieju

Copy link
Copy Markdown
Member

Thanks for the detailed writeup — the analysis checks out. I verified the layouts against v8/include/v8-script.h:544-607 and the static_assert in src/binding.cc:70.

On armv7 the C++ Source is 72 bytes / align 8 (11 leading fields = 44 B, 4 B padding, then 24 B of CompilationDetails), while the Rust struct was 56 bytes / align 4. Since v8__ScriptCompiler__Source__CONSTRUCT placement-news the real C++ object into the caller's MaybeUninit<Source>, that's a genuine stack buffer overflow. The fix also corrects a second issue you didn't mention: the old struct's alignment of 4 meant the int64_t stores into compilation_details could be misaligned even with a big enough buffer.

On 64-bit targets usize == u64, so this is a literal no-op — which makes it easy to take even though 32-bit isn't a CI target.

Two requests:

  1. Could you add a comment above the field so a future edit doesn't regress it back to usize?
// v8::ScriptCompiler::CompilationDetails: InMemoryCacheResult + 2 × int64_t.
// Must be u64, not usize — on 32-bit targets usize would undersize and
// under-align this struct, and the C++ constructor writes past the end.
_compilation_details: [u64; 3],
  1. Would you be up for pinning the Rust side to the C++ size? Nothing currently checks it — binding.cc asserts the C++ layout, but the Rust mirror is unverified, which is why this only surfaced at runtime on an untested arch. Exporting a v8__ScriptCompiler__Source__SIZEOF() and asserting against it in a test would catch this whole class of bug at build time. There's precedent in src/isolate_create_params.rs:344. Happy to take this as a follow-up PR instead if you'd rather keep this one minimal.

One caveat for the record: i686 Linux is still mismatched, since the i386 SysV ABI gives int64_t 4-byte alignment while Rust's i686-unknown-linux-gnu keeps u64 at 8. That's not a regression from this PR and it already fails loudly — align_to<int64_t> in binding.cc uses sizeof, not alignof, so that static_assert fails to compile on i386 regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants