fix(script): on_message UB + feat(session): compile_script + create_script_from_bytes#239
Merged
Conversation
…rida#189) Pre-fix, `handle_message` passed `*mut CallbackHandler` as the `g_signal_connect_data` user_data, but `call_on_message::<I>` re-cast that pointer to `*mut I` and dereferenced it: let handler: &mut I = &mut *(user_data as *mut I); handler.on_message(formatted_msg, data_vec); This reads I-sized bytes from the start of `CallbackHandler` (the `channel: (Sender, Receiver)` tuple) and treats them as `I`. It is undefined behavior, masked only when `I` is a ZST (e.g. the doc example's `struct Handler;`). Symptoms reported in frida#189: - `Arc<Mutex<_>>` handlers crash inside atomic_load. - Field reads return garbage / refcounts spike (e.g. 1624879470736). - Poisoned-mutex errors. For a handler whose fields include a `parking_lot`/`std::sync::Mutex`, the spurious atomic op on what frida-core treats as `Sender` internals can also deadlock `script.load_sync` outright if the JS script body issues a synchronous `send()` — the bogus SRWLOCK address looks held and `AcquireSRWLockExclusive` never returns. Fix: `user_data` is already `*mut CallbackHandler`, and `handle_message` stores the boxed trait object in `cb.script_handler`. Use that field instead of the type-pun. The `I` type parameter is kept (and given a `PhantomData::<I>` no-op) so monomorphization still emits a distinct fn pointer per handler type, preserving the original ABI for any external code that relied on it. Regression test (downstream): a `ScriptHandler` whose state includes a `u64` counter must increment cleanly from 1, 2, 3, ... rather than starting at a garbage value and corrupting adjacent memory.
frida-core has shipped a bytecode path for years: frida_session_compile_script_sync(session, source, opts) -> GBytes frida_session_create_script_from_bytes_sync(session, bytes, opts) -> FridaScript The Python and Node bindings expose both. frida-rust 0.17.2 only wraps the source-string path through `create_script`, leaving callers to either drop into frida-sys themselves or recompile on every load. This adds two thin methods on `Session`: fn compile_script(&self, source, &mut opts) -> Result<Vec<u8>> fn create_script_from_bytes<'b>(&self, bytes, &mut opts) -> Result<Script<'b>> `compile_script` copies the returned GBytes into an owned `Vec<u8>` and `g_bytes_unref`s the original. `create_script_from_bytes` wraps `g_bytes_new` / `g_bytes_unref` around the C call. Bytecode is runtime-specific — callers must compile and load with matching `ScriptOption::set_runtime`. Documented inline. Use cases this unblocks: - Startup speedup: skip the JS parse pass at every attach. - Soft script protection: ship .bin artifacts instead of readable JS. - Self-attach `compile_script` workflows so build pipelines can emit bytecode without spinning up an extra process.
Demonstrates the new Session::compile_script and Session::create_script_from_bytes pair from 6a92b72: source JS -> compile_script -> Vec<u8> bytecode -> write to compiled.bin (ship-as-artifact) -> read back -> create_script_from_bytes -> Script -> load Attaches to self (pid=0) so it runs out of the box with no external target, and pins ScriptRuntime::QJS on both ends to make the runtime-specific-bytecode constraint explicit.
Contributor
|
Please add tests so we can verify that we don't regress. |
…pt_from_bytes Covers three regression surfaces for 6a92b72: 1. compile_script returns a non-empty Vec<u8> and the result loads back through create_script_from_bytes (full QJS roundtrip via attach-to-self / pid=0). 2. compile_script with an interior-NUL source returns Error::CStringFailed without reaching the C side. 3. Both new methods work with bare ScriptOption::default(), matching the contract of create_script. CI already runs 'cargo test --features=auto-download' on the frida crate across Linux/macOS/Windows x x86/x86_64, so these run on every PR without workflow changes.
The merge in 5fe8f72 picked up parts of frida#235 (the `payload.get(N)` accessors in Exports::call, and the type change from `SendPayload` to `serde_json::Value`) but dropped two hunks: 1. The deletion of the now-dead `SendPayload` struct. 2. The accessor rewrite inside `call_on_message`, which still read `msg.payload.r#type`. After the type change this no longer compiled (E0609). Re-apply both hunks so the file matches what frida#235 intended: - Remove `SendPayload` (no remaining references in-tree). - Match the rpc envelope via `msg.payload.get(0).and_then(...)`, consistent with the same pattern at lines 367-368.
cargo test runs tests in parallel by default. Two threads doing concurrent device.attach(0) / detach() inside the same process corrupts frida-core's agent state — on Windows the test binary exits with STATUS_STACK_BUFFER_OVERRUN (0xc0000409); on other platforms it would be similarly undefined. Add a static Mutex<()> that every #[test] in this file acquires for the full attach -> detach span. unwrap_or_else(into_inner) recovers a poisoned lock so a panic in one test does not cascade and turn the rest of the suite red.
Contributor
|
Thanks! |
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
2 participants
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.
This branch bundles three related script + session improvements that have been in downstream use (the fridge wrapper, plus its Tauri / TUI consumers) for a while.
1. fix(script): on_message handler UB
ScriptHandler::on_messagewas routed through a callback that did not retain ownership of the user-data pointer correctly, so the handler could be dropped while frida-core still held a raw pointer to it. The dispatch is rerouted throughCallbackHandler.script_handler, removing the use-after-free.Originally filed as #189 but the patch there was never merged; this version is rebased on current main and adopts a regression-safe construction order.
2. feat(session): wrap
compile_scriptExposes
frida_session_compile_script_sync. Returns the bytecode blob so callers can compile once at build or startup, ship the bytes, and skip the JS-runtime parse cost on every attach.3. feat(session):
Session::create_script_from_bytesThe companion to (2). Loads precompiled bytecode instead of source — wraps
frida_session_create_script_from_bytes_sync.4. docs(examples):
compile_scriptround-tripexamples/core/compile_script/walks the matched API pair: compile a source string into bytes, then load those bytes into a fresh session.Compatibility
Session::create_script(source, ...)is unchanged.Tested
In daily use through fridge + frida-inspector + frida-wechat-tui (Tauri + TUI inspectors built on top of this fork). The UB fix in (1) has been load-tested against high-frequency message floods (Weixin protobuf hooks at thousands of messages per second) without the original double-free.