From bb54d551acc65e90b8c0acd909be13202fe1aaaa Mon Sep 17 00:00:00 2001 From: Dominik Maier Date: Thu, 4 Mar 2021 19:11:36 +0100 Subject: [PATCH] included more clippy suggestions --- fuzzers/libfuzzer_libmozjpeg/src/fuzzer.rs | 7 +- fuzzers/libfuzzer_libpng/src/fuzzer.rs | 8 +- libafl/src/bolts/llmp.rs | 102 +++++++++++---------- libafl/src/bolts/ownedref.rs | 24 +++-- libafl/src/bolts/shmem.rs | 8 +- libafl/src/corpus/mod.rs | 5 + libafl/src/observers/map.rs | 22 ++--- libafl/src/utils.rs | 22 +++-- 8 files changed, 104 insertions(+), 94 deletions(-) diff --git a/fuzzers/libfuzzer_libmozjpeg/src/fuzzer.rs b/fuzzers/libfuzzer_libmozjpeg/src/fuzzer.rs index 7e7e18dd89..664f8ae46b 100644 --- a/fuzzers/libfuzzer_libmozjpeg/src/fuzzer.rs +++ b/fuzzers/libfuzzer_libmozjpeg/src/fuzzer.rs @@ -86,10 +86,9 @@ fn fuzz(corpus_dirs: Vec, objective_dir: PathBuf, broker_port: u16) -> .expect("Failed to setup the restarter".into()); // Create an observation channel using the coverage map - let edges_observer = - StdMapObserver::new_from_ptr("edges", unsafe { __lafl_edges_map }, unsafe { - __lafl_max_edges_size as usize - }); + let edges_observer = unsafe { + StdMapObserver::new_from_ptr("edges", __lafl_edges_map, __lafl_max_edges_size as usize) + }; // If not restarting, create a State from scratch let mut state = state.unwrap_or_else(|| { diff --git a/fuzzers/libfuzzer_libpng/src/fuzzer.rs b/fuzzers/libfuzzer_libpng/src/fuzzer.rs index 90bb14ce5a..5c9e1e1489 100644 --- a/fuzzers/libfuzzer_libpng/src/fuzzer.rs +++ b/fuzzers/libfuzzer_libpng/src/fuzzer.rs @@ -97,11 +97,9 @@ fn fuzz(corpus_dirs: Vec, objective_dir: PathBuf, broker_port: u16) -> }; // Create an observation channel using the coverage map - let edges_observer = HitcountsMapObserver::new(StdMapObserver::new_from_ptr( - "edges", - unsafe { __lafl_edges_map }, - unsafe { __lafl_max_edges_size as usize }, - )); + let edges_observer = HitcountsMapObserver::new(unsafe { + StdMapObserver::new_from_ptr("edges", __lafl_edges_map, __lafl_max_edges_size as usize) + }); // If not restarting, create a State from scratch let mut state = state.unwrap_or_else(|| { diff --git a/libafl/src/bolts/llmp.rs b/libafl/src/bolts/llmp.rs index aad249cf40..df975c8088 100644 --- a/libafl/src/bolts/llmp.rs +++ b/libafl/src/bolts/llmp.rs @@ -976,61 +976,59 @@ where }; // Let's see what we go here. - match ret { - Some(msg) => { - if !(*msg).in_map(&mut self.current_recv_map) { - return Err(Error::IllegalState("Unexpected message in map (out of map bounds) - bugy client or tampered shared map detedted!".into())); - } - // Handle special, LLMP internal, messages. - match (*msg).tag { - LLMP_TAG_UNSET => panic!("BUG: Read unallocated msg"), - LLMP_TAG_END_OF_PAGE => { - #[cfg(feature = "std")] - dbg!("Got end of page, allocing next"); - // Handle end of page - if (*msg).buf_len < size_of::() as u64 { - panic!( - "Illegal message length for EOP (is {}, expected {})", - (*msg).buf_len_padded, - size_of::() - ); - } - let pageinfo = (*msg).buf.as_mut_ptr() as *mut LlmpPayloadSharedMapInfo; - - /* We can reuse the map mem space, no need to free and calloc. - However, the pageinfo points to the map we're about to unmap. - Clone the contents first to be safe (probably fine in rust eitner way). */ - let pageinfo_cpy = (*pageinfo).clone(); - - // Mark the old page save to unmap, in case we didn't so earlier. - ptr::write_volatile(&mut (*page).save_to_unmap, 1); - // Map the new page. The old one should be unmapped by Drop - self.current_recv_map = - LlmpSharedMap::existing(SH::existing_from_shm_slice( - &pageinfo_cpy.shm_str, - pageinfo_cpy.map_size, - )?); - // Mark the new page save to unmap also (it's mapped by us, the broker now) - ptr::write_volatile(&mut (*page).save_to_unmap, 1); - - #[cfg(feature = "std")] - dbg!("Got a new recv map", self.current_recv_map.shmem.shm_str()); - // After we mapped the new page, return the next message, if available - return self.recv(); - } - _ => (), - } - - // Store the last msg for next time - self.last_msg_recvd = msg; + if let Some(msg) = ret { + if !(*msg).in_map(&mut self.current_recv_map) { + return Err(Error::IllegalState("Unexpected message in map (out of map bounds) - bugy client or tampered shared map detedted!".into())); } - _ => (), + // Handle special, LLMP internal, messages. + match (*msg).tag { + LLMP_TAG_UNSET => panic!("BUG: Read unallocated msg"), + LLMP_TAG_END_OF_PAGE => { + #[cfg(feature = "std")] + dbg!("Got end of page, allocing next"); + // Handle end of page + if (*msg).buf_len < size_of::() as u64 { + panic!( + "Illegal message length for EOP (is {}, expected {})", + (*msg).buf_len_padded, + size_of::() + ); + } + let pageinfo = (*msg).buf.as_mut_ptr() as *mut LlmpPayloadSharedMapInfo; + + /* We can reuse the map mem space, no need to free and calloc. + However, the pageinfo points to the map we're about to unmap. + Clone the contents first to be safe (probably fine in rust eitner way). */ + let pageinfo_cpy = (*pageinfo).clone(); + + // Mark the old page save to unmap, in case we didn't so earlier. + ptr::write_volatile(&mut (*page).save_to_unmap, 1); + // Map the new page. The old one should be unmapped by Drop + self.current_recv_map = LlmpSharedMap::existing(SH::existing_from_shm_slice( + &pageinfo_cpy.shm_str, + pageinfo_cpy.map_size, + )?); + // Mark the new page save to unmap also (it's mapped by us, the broker now) + ptr::write_volatile(&mut (*page).save_to_unmap, 1); + + #[cfg(feature = "std")] + dbg!("Got a new recv map", self.current_recv_map.shmem.shm_str()); + // After we mapped the new page, return the next message, if available + return self.recv(); + } + _ => (), + } + + // Store the last msg for next time + self.last_msg_recvd = msg; }; Ok(ret) } /// Blocks/spins until the next message gets posted to the page, /// then returns that message. + /// # Safety + /// Returns a raw ptr, on the recv map. Should be safe in general pub unsafe fn recv_blocking(&mut self) -> Result<*mut LlmpMsg, Error> { let mut current_msg_id = 0; let page = self.current_recv_map.page_mut(); @@ -1774,6 +1772,8 @@ where } /// Commits a msg to the client's out map + /// # Safety + /// Needs to be called with a proper msg pointer pub unsafe fn send(&mut self, msg: *mut LlmpMsg) -> Result<(), Error> { self.sender.send(msg) } @@ -1804,6 +1804,8 @@ where /// A client receives a broadcast message. /// Returns null if no message is availiable + /// # Safety + /// Should be save, unless the internal state is corrupt. Returns raw ptr. #[inline] pub unsafe fn recv(&mut self) -> Result, Error> { self.receiver.recv() @@ -1811,6 +1813,8 @@ where /// A client blocks/spins until the next message gets posted to the page, /// then returns that message. + /// # Safety + /// Should be save, unless the internal state is corrupt. Returns raw ptr. #[inline] pub unsafe fn recv_blocking(&mut self) -> Result<*mut LlmpMsg, Error> { self.receiver.recv_blocking() @@ -1857,7 +1861,7 @@ where LLMP_PREF_INITIAL_MAP_SIZE, )?))?; - stream.write(ret.sender.out_maps.first().unwrap().shmem.shm_slice())?; + stream.write_all(ret.sender.out_maps.first().unwrap().shmem.shm_slice())?; Ok(ret) } } diff --git a/libafl/src/bolts/ownedref.rs b/libafl/src/bolts/ownedref.rs index 82b6bef9ab..968b7bc4ce 100644 --- a/libafl/src/bolts/ownedref.rs +++ b/libafl/src/bolts/ownedref.rs @@ -31,8 +31,8 @@ where } } -impl<'a, T: Sized> Ptr<'a, T> { - pub fn as_ref(&self) -> &T { +impl<'a, T: Sized> AsRef for Ptr<'a, T> { + fn as_ref(&self) -> &T { match self { Ptr::Ref(r) => r, Ptr::Owned(v) => v.as_ref(), @@ -69,15 +69,17 @@ where } } -impl<'a, T: Sized> PtrMut<'a, T> { - pub fn as_ref(&self) -> &T { +impl<'a, T: Sized> AsRef for PtrMut<'a, T> { + fn as_ref(&self) -> &T { match self { PtrMut::Ref(r) => r, PtrMut::Owned(v) => v.as_ref(), } } +} - pub fn as_mut(&mut self) -> &T { +impl<'a, T: Sized> AsMut for PtrMut<'a, T> { + fn as_mut(&mut self) -> &mut T { match self { PtrMut::Ref(r) => r, PtrMut::Owned(v) => v.as_mut(), @@ -195,8 +197,8 @@ where } } -impl Cptr { - pub fn as_ref(&self) -> &T { +impl AsRef for Cptr { + fn as_ref(&self) -> &T { match self { Cptr::Cptr(p) => unsafe { p.as_ref().unwrap() }, Cptr::Owned(v) => v.as_ref(), @@ -230,15 +232,17 @@ where } } -impl CptrMut { - pub fn as_ref(&self) -> &T { +impl AsRef for CptrMut { + fn as_ref(&self) -> &T { match self { CptrMut::Cptr(p) => unsafe { p.as_ref().unwrap() }, CptrMut::Owned(b) => b.as_ref(), } } +} - pub fn as_mut(&mut self) -> &mut T { +impl AsMut for CptrMut { + fn as_mut(&mut self) -> &mut T { match self { CptrMut::Cptr(p) => unsafe { p.as_mut().unwrap() }, CptrMut::Owned(b) => b.as_mut(), diff --git a/libafl/src/bolts/shmem.rs b/libafl/src/bolts/shmem.rs index 95c88c04b1..a4a4d24436 100644 --- a/libafl/src/bolts/shmem.rs +++ b/libafl/src/bolts/shmem.rs @@ -169,7 +169,7 @@ pub mod unix_shmem { #[cfg(target_os = "android")] unsafe fn shmctl(__shmid: c_int, __cmd: c_int, _buf: *mut shmid_ds) -> c_int { - print!("shmctl(__shmid: {})\n", __shmid); + println!("shmctl(__shmid: {})", __shmid); if __cmd == 0 { let length = ioctl(__shmid, ASHMEM_GET_SIZE); @@ -199,7 +199,7 @@ pub mod unix_shmem { __key, ); - print!("ourkey: {:?}\n", ourkey); + println!("ourkey: {:?}", ourkey); if ioctl(fd, ASHMEM_SET_NAME, &ourkey) != 0 { close(fd); return 0; @@ -210,13 +210,13 @@ pub mod unix_shmem { return 0; }; - print!("shmget returns {}\n", fd); + println!("shmget returns {}", fd); fd } #[cfg(target_os = "android")] unsafe fn shmat(__shmid: c_int, __shmaddr: *const c_void, __shmflg: c_int) -> *mut c_void { - print!("shmat(__shmid: {})\n", __shmid); + println!("shmat(__shmid: {})", __shmid); let size = ioctl(__shmid, ASHMEM_GET_SIZE); if size < 0 { diff --git a/libafl/src/corpus/mod.rs b/libafl/src/corpus/mod.rs index 7e9baa3a53..3f48ffa0c7 100644 --- a/libafl/src/corpus/mod.rs +++ b/libafl/src/corpus/mod.rs @@ -39,6 +39,11 @@ where /// Returns the number of elements fn count(&self) -> usize; + /// Returns true, if no elements are in this corpus yet + fn is_empty(&self) -> bool { + self.count() == 0 + } + /// Add an entry to the corpus and return its index fn add(&mut self, testcase: Testcase) -> Result; diff --git a/libafl/src/observers/map.rs b/libafl/src/observers/map.rs index 2a82ade8c8..141f75f02d 100644 --- a/libafl/src/observers/map.rs +++ b/libafl/src/observers/map.rs @@ -127,12 +127,10 @@ where } /// Creates a new MapObserver from a raw pointer - pub fn new_from_ptr(name: &'static str, map_ptr: *mut T, len: usize) -> Self { - let initial = if len > 0 { - unsafe { *map_ptr } - } else { - T::default() - }; + /// # Safety + /// Will dereference the map_ptr with up to len elements. + pub unsafe fn new_from_ptr(name: &'static str, map_ptr: *mut T, len: usize) -> Self { + let initial = if len > 0 { *map_ptr } else { T::default() }; StdMapObserver { map: ArrayMut::Cptr((map_ptr, len)), name: name.to_string(), @@ -215,7 +213,7 @@ where { /// Creates a new MapObserver pub fn new(name: &'static str, map: &'static mut [T], size: &usize) -> Self { - let initial = if map.len() > 0 { map[0] } else { T::default() }; + let initial = if map.is_empty() { T::default() } else { map[0] }; Self { map: ArrayMut::Cptr((map.as_mut_ptr(), map.len())), size: Cptr::Cptr(size as *const _), @@ -225,17 +223,15 @@ where } /// Creates a new MapObserver from a raw pointer - pub fn new_from_ptr( + /// # Safety + /// Dereferences map_ptr with up to max_len elements of size_ptr. + pub unsafe fn new_from_ptr( name: &'static str, map_ptr: *mut T, max_len: usize, size_ptr: *const usize, ) -> Self { - let initial = if max_len > 0 { - unsafe { *map_ptr } - } else { - T::default() - }; + let initial = if max_len > 0 { *map_ptr } else { T::default() }; VariableMapObserver { map: ArrayMut::Cptr((map_ptr, max_len)), size: Cptr::Cptr(size_ptr), diff --git a/libafl/src/utils.rs b/libafl/src/utils.rs index c1f655d6a1..bcd90fd07d 100644 --- a/libafl/src/utils.rs +++ b/libafl/src/utils.rs @@ -4,10 +4,10 @@ use core::{cell::RefCell, debug_assert, fmt::Debug, time}; use serde::{de::DeserializeOwned, Deserialize, Serialize}; use xxhash_rust::xxh3::xxh3_64_with_seed; -#[cfg(unix)] -use alloc::string::ToString; #[cfg(unix)] use libc::pid_t; +#[cfg(unix)] +use std::ffi::CString; use crate::Error; @@ -415,15 +415,19 @@ pub enum ForkResult { } /// Unix has forks. +/// # Safety +/// A Normal fork. Runs on in two processes. Should be memory safe in general. #[cfg(unix)] pub unsafe fn fork() -> Result { - let pid = libc::fork(); - if pid < 0 { - Err(Error::Unknown("Fork failed".to_string())) - } else if pid == 0 { - Ok(ForkResult::Child) - } else { - Ok(ForkResult::Parent(ChildHandle { pid })) + match libc::fork() { + pid if pid > 0 => Ok(ForkResult::Parent(ChildHandle { pid })), + pid if pid < 0 => { + // Getting errno from rust is hard, we'll just let the libc print to stderr for now. + // In any case, this should usually not happen. + libc::perror(CString::new("Fork failed").unwrap().as_ptr()); + Err(Error::Unknown(format!("Fork failed ({})", pid))) + } + _ => Ok(ForkResult::Child), } }