这是indexloc提供的服务,不要输入任何密码
Skip to content

Fix unsound with_task_id_mapping implementation #2362

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

Merged
merged 1 commit into from
Oct 28, 2022

Conversation

bdbai
Copy link
Contributor

@bdbai bdbai commented Oct 26, 2022

Currently in with_task_id_mapping, the ID mapping object with any lifetime is promoted to 'static through unsafe function std::men::transmute. It actually assumes the object could not outlive the function, however it is possible when a panic throws in the callback, possibly causing a use-after-free from safe code.

The following code snippet exhibits the undefined behaviour:

#[cfg(test)]
 mod tests {
     use crate::id::{with_task_id_mapping, IdMapping, TaskId};

     struct IdentIdMapping<'a>(&'a usize);
     impl<'a> IdMapping<TaskId> for IdentIdMapping<'a> {
         fn forward(&self, _id: TaskId) -> usize {
             self.0 + 1
         }
         fn backward(&self, id: usize) -> TaskId {
             id.into()
         }
     }

     #[test]
     fn with_task_id_mapping_panic() {
         let _ = std::panic::catch_unwind(|| {
             let tid = 5;
             let m = IdentIdMapping(&tid);
             with_task_id_mapping(m, || panic!("with_task_id_mapping_panic"))
         });
         let tid: TaskId = 4.into();
         println!("{:?}", serde_json::to_string(&tid));
     }
 }

bdbai@51f4a43

Running cargo miri test -p turbo-tasks with_task_id_mapping_panic gives:

test id::tests::with_task_id_mapping_panic ... error: Undefined Behavior: constructing invalid value: encountered a dangling reference (use-after-free)
   --> crates/turbo-tasks/src/id.rs:237:13
    |
237 |             self.0 + 1
    |             ^^^^^^ constructing invalid value: encountered a dangling reference (use-after-free)
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `<id::tests::IdentIdMapping as id::IdMapping<id::TaskId>>::forward` at crates/turbo-tasks/src/id.rs:237:13
note: inside closure at crates/turbo-tasks/src/id.rs:187:17
   --> crates/turbo-tasks/src/id.rs:187:17
    |
187 |                 mapping.forward(*self)
    |                 ^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `std::thread::LocalKey::<std::cell::RefCell<std::option::Option<std::boxed::Box<dyn id::IdMapping<id::TaskId>>>>>::try_with::<[closure@crates/turbo-tasks/src/id.rs:185:30: 185:36], std::result::Result<(), serde_json::Error>>` at /Users/bdbai/.rustup/toolchains/nightly-2022-09-23-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:445:16
    = note: inside `std::thread::LocalKey::<std::cell::RefCell<std::option::Option<std::boxed::Box<dyn id::IdMapping<id::TaskId>>>>>::with::<[closure@crates/turbo-tasks/src/id.rs:185:30: 185:36], std::result::Result<(), serde_json::Error>>` at /Users/bdbai/.rustup/toolchains/nightly-2022-09-23-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:421:9
note: inside `<id::TaskId as backend::_::_serde::Serialize>::serialize::<&mut serde_json::Serializer<&mut std::vec::Vec<u8>>>` at crates/turbo-tasks/src/id.rs:185:9
   --> crates/turbo-tasks/src/id.rs:185:9
    |
185 | /         TASK_ID_MAPPING.with(|cell| {
186 | |             let mapped_id = if let Some(mapping) = cell.borrow().as_ref() {
187 | |                 mapping.forward(*self)
188 | |             } else {
...   |
191 | |             serializer.serialize_u64(mapped_id as u64)
192 | |         })
    | |__________^
    = note: inside `serde_json::to_writer::<&mut std::vec::Vec<u8>, id::TaskId>` at /Users/bdbai/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_json-1.0.85/src/ser.rs:2155:10
    = note: inside `serde_json::to_vec::<id::TaskId>` at /Users/bdbai/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_json-1.0.85/src/ser.rs:2190:10
    = note: inside `serde_json::to_string::<id::TaskId>` at /Users/bdbai/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_json-1.0.85/src/ser.rs:2221:20
note: inside `id::tests::with_task_id_mapping_panic` at crates/turbo-tasks/src/id.rs:252:26
   --> crates/turbo-tasks/src/id.rs:252:26
    |
252 |         println!("{:?}", serde_json::to_string(&tid));
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at crates/turbo-tasks/src/id.rs:245:5
   --> crates/turbo-tasks/src/id.rs:245:5
    |
244 |       #[test]
    |       ------- in this procedural macro expansion
245 | /     fn with_task_id_mapping_panic() {
246 | |         let _ = std::panic::catch_unwind(|| {
247 | |             let tid = 5;
248 | |             let m = IdentIdMapping(&tid);
...   |
252 | |         println!("{:?}", serde_json::to_string(&tid));
253 | |     }
    | |_____^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error; 1 warning emitted

error: test failed, to rerun pass `-p turbo-tasks --lib`

The fix uses an RAII guard to ensure the temporary assignment is rolled back even when func panics.

@vercel
Copy link

vercel bot commented Oct 26, 2022

Someone is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

5 Ignored Deployments
Name Status Preview Updated
examples-basic-web ⬜️ Ignored (Inspect) Oct 26, 2022 at 8:11AM (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Oct 26, 2022 at 8:11AM (UTC)
examples-kitchensink-blog ⬜️ Ignored (Inspect) Oct 26, 2022 at 8:11AM (UTC)
examples-native-web ⬜️ Ignored (Inspect) Oct 26, 2022 at 8:11AM (UTC)
examples-svelte-web ⬜️ Ignored (Inspect) Oct 26, 2022 at 8:11AM (UTC)

@bdbai bdbai changed the title Fix unsound with_task_id_mapping implementation Fix unsound with_task_id_mapping implementation Oct 26, 2022
@jridgewell jridgewell merged commit 3423b39 into vercel:main Oct 28, 2022
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
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