Fix as_object UB discussed in #1748
#1751
Conversation
There was a problem hiding this comment.
Definitely an improvement, though I'm still concerned about inter-{thread,process} data races as mentioned on #1748.
| @@ -147,6 +218,7 @@ impl<'a, T: Sized> AsMut<T> for OwnedRefMut<'a, T> { | |||
| #[must_use] | |||
| fn as_mut(&mut self) -> &mut T { | |||
| match self { | |||
| OwnedRefMut::RefRaw(r, _) => unsafe { r.as_mut().unwrap() }, | |||
There was a problem hiding this comment.
This PR is definitely of an improvement, in that we're now storing a raw pointer rather than a reference - this is important because holding a reference while the memory is mutated is UB.
However, it looks to me like it's still pretty easy to end up with UB by calling this method on the return value of ShMem::as_owned_ref_mut_of. as_{ref,mut} should probably be marked unsafe and have a comment stating that the memory can't be mutated (or even read, in the case of as_mut) while the returned reference is live, and point out that such mutation can happen via ShMem.
There was a problem hiding this comment.
My idea was that every possible constructor of OwnedRef::RefRaw is marked unsafe so that we don't need to mark all as unsafe. We cannot mark every use of the OwnedRef unsafe, since we want to implement the Serialize trait - and marking all of the serialization functions unsafe is out of our control.
There was a problem hiding this comment.
This is the reason I added UnsafeMarker - there is no legal way to create a RefRaw without calling an unsafe constructor first. That's the best we can do I fear.
There was a problem hiding this comment.
My idea was that every possible constructor of OwnedRef::RefRaw is marked unsafe so that we don't need to mark all as unsafe.
I see what you're saying. It's perhaps worth noting that the standard library takes the opposite approach - it's always safe to create raw pointers, always unsafe to dereference them.
There was a problem hiding this comment.
If we wouldn't need to serialize/deserialize it would be the way to go, but it's not an option here, sadly
There was a problem hiding this comment.
Do you mean we should add a comment to the Safety section here or something?
| @@ -215,7 +216,7 @@ macro_rules! fuzz_with { | |||
| // Create a stacktrace observer | |||
| let backtrace_observer = BacktraceObserver::new( | |||
| "BacktraceObserver", | |||
| unsafe { &mut BACKTRACE }, | |||
| OwnedRefMut::owned(None), | |||
There was a problem hiding this comment.
@addisoncrump can you doublecheck that this is correct? I didn't see any reason to use a static variable here(?) Nobody else seems to access it(?)
Would it be faster than having the value on the heap?
There was a problem hiding this comment.
Slow response here, sorry. IIRC that is an artifact of copy/pasting from an old example. This update seems reasonable.
* Fix as_object UB discussed in AFLplusplus#1748 * More cleanup, more less UB * Fix fixes * Added uninit_on_shmem api * clippy * fmt * trying to fix fuzzers, libfuzzer wrapper * Add OwnedRefMit::owned constructor, libfuzzer fix * Some more fixes * Add BacktaceObserver::owned fn * fmt * more fmt
* Added paging filtering. Reworked address range filtering to fit with new generic code. * Fix: renamed remaining QemuInstrumentationFilter instances. * Renamed sync breakpoint to sync exit. * Split emu in systemmode.rs / usermode.rs for specific code. EmuExitHandler implementation. * sync_backdoor.rs removal. Formatting. * Updated `bindgen` and `which`. Adapting code to work with update. * fix: reconfigure cleanly if prior configure was interrupted abruptly. * Enable sanitizers in QEMU during debug. * Added target-usable files. * Added breakpoint structure. * Adapted other files to work with ExitHandler. * Adapted existing fuzzer to work with new exit handler. * fix: use get to avoid crashes. * Updated README to indicate cargo-make should be installed. * Added QEMU internal exit handler. * Adapted qemu_systemmode example with new exit handler. * Fixed fuzzers to work with new exit handler. * Trying to fix CI (#1739) * test * dummy * dummy * Added new examples. * Forgot to add build scripts. * format * format * clang-format * python emulator adaptation. * fixed python bindings. * clippy fixes. * python bindings. * fix qemu_sugar. * fix fuzzbench. * fixed import issues. * misc fixes. * renamed crate. * Updated x86_64 stub bindings. * Fixed static naming. * binding fmt * clippy * clippy * Removed useless return statement. * removed advice to install cargo-make in individual repositories. * symcc_update (#1749) * Remove unused create_anymap_for_trait macro (fixes #1719) (#1752) * Fix `as_object` UB discussed in #1748 (#1751) * Fix as_object UB discussed in #1748 * More cleanup, more less UB * Fix fixes * Added uninit_on_shmem api * clippy * fmt * trying to fix fuzzers, libfuzzer wrapper * Add OwnedRefMit::owned constructor, libfuzzer fix * Some more fixes * Add BacktaceObserver::owned fn * fmt * more fmt * Ignore SigPipe by default (#1741) * Ignore SigPipe by default * Fix no_std * fmt * Fix incorrect imports (#1758) * Fix incorrect imports https://doc.rust-lang.org/core/simd/trait.SimdOrd.html * Fix * Try fix ci * Documentation fixes (#1761) * Documentation fixes * Fix InProcessExecutor url * Update all urls to latest * Miri ignores for M1 regex (#1762) * Enabling DrCov on Windows (#1765) * Enabling DrCov for Windows * Dedup common code in scheduler (#1702) * dedup common code in scheduler * del eco * fixing * fix * replace `Emulator::new_empty` by `Emulator::get` calls outside of `emu.rs` for safety. (#1763) * Add mute_inprocess_target fn, SimpleFdLogger::set_logger, and more (#1754) * Add mute_inprocess_target fn, SimpleFdLogger::set_logger, set_error_print_panic_hook * Trying to fix #1753 * typo * More fix * Fix test? * more testcase fixes * Fix: renamed remaining QemuInstrumentationFilter instances. * Split emu in systemmode.rs / usermode.rs for specific code. EmuExitHandler implementation. * format * format * format * Replace sync_exit with sync_backdoor. * Rework command system. * fix bad import. * format. * cargo fmt * disable af-xdp as well to avoid linking errors. * End of merging. * format. * Adaptation for usermode. * format. * injection support. * usermode fixes. format. * clippy * clippy + format * Do not unwrap emu + format. * fix: entry_point breakpoint * inital commit. * clippy * tests * clippy * adapt example * systemmode. * renaming * fmt * fix lints. * more lint fix. * even more lint fixes. * always more lint fixes. * lint fix. * allow unused qualifications for crate when it could be confusing. * Still lint fixes. * Lint fixes on generated code. * Some lint fixes. * merge continue. * renamed modules as well. * fixing merge. * systemmode compiling. * fmt * fix early emulator drop. * fmt * fix cast to c_void of the wrong object. * Added global enum for snapshot managers. Some renaming. * move things around. * WIP: generic inclusion of QEMU Executor in exit handler. * * Moved extern calls to `libafl_qemu_sys` * Replaced old `Emulator` by `Qemu` and only kept C functions wrappers * Now `Emulator` is for higher-level interactions with QEMU. Kept old functions for compatibility calling to `Qemu` functions * A direct side effect of this slit is the removal of the `IsEmuExitHandler` trait dependency added in many parts of the code. * Removed old dirty casting for `QemuExecutor` helpers and used the brand-new access to `QemuExecutorState` instead. * Minor changes to `Qemu` and `Emulator` `get` methods for cleaner getters. * Add missing `Qemu` function. * Updated `qemu_systemmode` example. * Adapted QEMU fuzzers + renaming. * Fixed python. * fix libafl_sugar with new implementation. * fix dangling RefCell. adapt new examples. TODO: merge `libafl_systemmode.*` examples. * clippy. * fix more fuzzers. * clippy. * Implement `HasInstrumentationFilter` generically. Updated `StdInstrumentationFilter` accordingly. * Renamed breakpoint functions for QEMU. `qemu.run()` handling. * Removed OnceCell / RefCell in signature. more explicit `MmapPerms` method names. * minor code refactoring * Emulator::run_handle refactoring * deprecated Emulator functions calling directly to QEMU functions. * IsSnapshotManager -> SnapshotManager * IsEmuExitHandler -> EmuExitHandler + fmt * Generic register when it makes sense. * reverted IsSnapshotManager -> SnapshotManager because of a collision. * fix syntax + clippy * fmt --------- Co-authored-by: Dongjia "toka" Zhang <tokazerkje@outlook.com> Co-authored-by: Dominik Maier <domenukk@gmail.com> Co-authored-by: lazymio <mio@lazym.io> Co-authored-by: Bet4 <0xbet4@gmail.com> Co-authored-by: mkravchik <mkravchik@hotmail.com>
This is an attempt to fix the undefined behaviour we currently have, as discussed in #1748