fast_api: require &'static for CFunction overload storage#1990
Open
divybot wants to merge 1 commit into
Open
Conversation
Upstream crrev.com/c/7828135 (slated for V8 15) changes FunctionTemplateInfo to store the raw v8::CFunction pointers passed to NewWithCFunctionOverloads directly, rather than copying them into a Managed<CFunctionWithSignature> heap object. The pointed-to storage must therefore outlive every FunctionTemplate that references it - in practice this means it has to be 'static, since a FunctionTemplate can live until isolate disposal. Enforce the invariant at the Rust type level so embedders cannot pass stack-local overload storage: * FunctionBuilder::build_fast now requires &'static [CFunction]. * CFunction::new now requires &'static CFunctionInfo so the type_info pointer it caches cannot dangle. * CFunctionInfo::new now requires &'static [CTypeInfo] for arg_info for the same reason - the constructed CFunctionInfo caches arg_info.as_ptr(). The existing call sites in tests/test_api.rs and benches/function.rs already use the canonical pattern - const FAST: CFunction = ...; build_fast( scope, &[FAST]) - which Rust static-promotes to &'static [CFunction; 1], so no test changes are required. A new trybuild compile_fail test (tests/compile_fail/build_fast_local_overloads.rs) demonstrates that a runtime-built Vec<CFunction> is now rejected with E0597. Refs #1989 Closes denoland/orchid#160 Co-Authored-By: Divy Srivastava <me@littledivy.com>
|
|
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
Resolves #1989 by enforcing — at the Rust type level — the
storage-lifetime invariant introduced by upstream V8 change
crrev.com/c/7828135
(
[fastapi] Store v8::CFunction pointer directly in FunctionTemplateInfo,slated for V8 15).
Before that V8 patch,
FunctionTemplate::NewWithCFunctionOverloadscopiedeach
v8::CFunctioninto aManaged<CFunctionWithSignature>heap object, sothe embedder's overload storage only needed to outlive the call. After it,
V8 retains the raw
v8::CFunction*directly insideFunctionTemplateInfoand reads it back on every fast-call dispatch, so the pointed-to storage
must outlive every
FunctionTemplatethat references it — which inpractice means
'static(templates can live until isolate disposal).Today
FunctionBuilder::build_fastaccepts any&[CFunction]and forwardsas_ptr()to V8, so a deno_core-style call shape likebuilder.build_fast(scope, &[fast_function])would silently dangle once theV8 15 ABI lands. This PR makes that misuse a compile error today instead
of a use-after-free tomorrow.
Changes
FunctionBuilder::build_fastnow takesoverloads: &'static [CFunction].fast_api::CFunction::newnow takestype_info: &'static CFunctionInfo,so the
type_info_pointer it caches cannot dangle either.fast_api::CFunctionInfo::newnow takesarg_info: &'static [CTypeInfo],same reason — the constructed
CFunctionInfocachesarg_info.as_ptr().static-promoted
constpattern.Tests / call sites
The existing call sites in
tests/test_api.rsandbenches/function.rsalready use the canonical pattern —
— which Rust static-promotes to
&'static [CFunction; 1], so no testchanges are required.
cargo check --benchesandcargo test --test test_uiboth pass locally; the test-suitetest_api/test_cppgcbinaries also compile through the relevant call sites (their only
remaining errors here are missing
third_party/icu/common/icudtl.dat,unrelated to this change).
A new trybuild compile_fail test
tests/compile_fail/build_fast_local_overloads.rsdemonstrates that a runtime-built
Vec<CFunction>is now rejected withE0597 — argument requires that 'overloads' is borrowed for 'static.Context
Closes denoland/divybot#160