From c404825fb8c676b40c8747d86731158b442533f6 Mon Sep 17 00:00:00 2001 From: Dominik Maier Date: Sun, 22 May 2022 13:01:55 +0200 Subject: [PATCH] More clippy (#641) * Even more libafl_frida clippy * Eq * addr_of_mut cleanup * fmt --- libafl/src/bolts/llmp.rs | 2 +- libafl/src/bolts/os/unix_shmem_server.rs | 2 +- libafl/src/events/llmp.rs | 4 +- libafl/src/events/mod.rs | 2 +- libafl/src/executors/timeout.rs | 9 +- libafl/src/fuzzer/mod.rs | 2 +- libafl/src/lib.rs | 2 +- libafl/src/mutators/mod.rs | 2 +- libafl/src/schedulers/powersched.rs | 2 +- libafl_frida/src/asan/asan_rt.rs | 6 +- libafl_frida/src/cmplog_rt.rs | 158 ++++++++++++----------- libafl_frida/src/coverage_rt.rs | 2 +- libafl_frida/src/helper.rs | 12 +- libafl_qemu/src/emu.rs | 2 +- 14 files changed, 114 insertions(+), 93 deletions(-) diff --git a/libafl/src/bolts/llmp.rs b/libafl/src/bolts/llmp.rs index e3a84abca9..4746b2a747 100644 --- a/libafl/src/bolts/llmp.rs +++ b/libafl/src/bolts/llmp.rs @@ -1444,7 +1444,7 @@ where LLMP_TAG_EXITING => { // The other side is done. assert_eq!((*msg).buf_len, 0); - return Err(Error::shuttingdown()); + return Err(Error::shutting_down()); } LLMP_TAG_END_OF_PAGE => { #[cfg(feature = "std")] diff --git a/libafl/src/bolts/os/unix_shmem_server.rs b/libafl/src/bolts/os/unix_shmem_server.rs index aedbecd0a7..6fc92da10e 100644 --- a/libafl/src/bolts/os/unix_shmem_server.rs +++ b/libafl/src/bolts/os/unix_shmem_server.rs @@ -571,7 +571,7 @@ where ServedShMemRequest::Exit => { println!("ShMemService - Exiting"); // stopping the server - return Err(Error::shuttingdown()); + return Err(Error::shutting_down()); } }; // println!("send ashmem client: {}, response: {:?}", client_id, &response); diff --git a/libafl/src/events/llmp.rs b/libafl/src/events/llmp.rs index 99d089b0c3..870218cb50 100644 --- a/libafl/src/events/llmp.rs +++ b/libafl/src/events/llmp.rs @@ -789,7 +789,7 @@ where broker_things(event_broker, self.remote_broker_addr)?; - return Err(Error::shuttingdown()); + return Err(Error::shutting_down()); } LlmpConnection::IsClient { client } => { let mgr = @@ -807,7 +807,7 @@ where broker_things(event_broker, self.remote_broker_addr)?; - return Err(Error::shuttingdown()); + return Err(Error::shutting_down()); } ManagerKind::Client { cpu_core } => { // We are a client diff --git a/libafl/src/events/mod.rs b/libafl/src/events/mod.rs index fd049a5872..1850a0560e 100644 --- a/libafl/src/events/mod.rs +++ b/libafl/src/events/mod.rs @@ -73,7 +73,7 @@ pub enum BrokerEventResult { } /// Distinguish a fuzzer by its config -#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)] pub enum EventConfig { /// Always assume unique setups for fuzzer configs AlwaysUnique, diff --git a/libafl/src/executors/timeout.rs b/libafl/src/executors/timeout.rs index fe82ac8e7f..1110d4f0c3 100644 --- a/libafl/src/executors/timeout.rs +++ b/libafl/src/executors/timeout.rs @@ -39,7 +39,10 @@ use windows::Win32::{ use core::{ffi::c_void, ptr::write_volatile}; #[cfg(windows)] -use core::sync::atomic::{compiler_fence, Ordering}; +use core::{ + addr_of_mut, + sync::atomic::{compiler_fence, Ordering}, +}; #[repr(C)] #[cfg(all(unix, not(target_os = "linux")))] @@ -235,7 +238,7 @@ impl TimeoutExecutor { let tp_timer = unsafe { CreateThreadpoolTimer( Some(timeout_handler), - core::ptr::addr_of_mut!(GLOBAL_STATE) as *mut c_void, + addr_of_mut!(GLOBAL_STATE) as *mut c_void, &TP_CALLBACK_ENVIRON_V3::default(), ) }; @@ -284,7 +287,7 @@ where write_volatile(&mut data.tp_timer, self.tp_timer as *mut _ as *mut c_void); write_volatile( &mut data.critical, - core::ptr::addr_of_mut!(self.critical) as *mut c_void, + addr_of_mut!(self.critical) as *mut c_void, ); write_volatile( &mut data.timeout_input_ptr, diff --git a/libafl/src/fuzzer/mod.rs b/libafl/src/fuzzer/mod.rs index fd6a0077fc..be9b538264 100644 --- a/libafl/src/fuzzer/mod.rs +++ b/libafl/src/fuzzer/mod.rs @@ -222,7 +222,7 @@ where } /// The corpus this input should be added to -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Eq)] pub enum ExecuteInputResult { /// No special input None, diff --git a/libafl/src/lib.rs b/libafl/src/lib.rs index 7fa263422c..93ce650f43 100644 --- a/libafl/src/lib.rs +++ b/libafl/src/lib.rs @@ -243,7 +243,7 @@ impl Error { } /// Shutting down, not really an error. #[must_use] - pub fn shuttingdown() -> Self { + pub fn shutting_down() -> Self { Error::ShuttingDown } /// Something else happened diff --git a/libafl/src/mutators/mod.rs b/libafl/src/mutators/mod.rs index c8901988bb..4b222314bb 100644 --- a/libafl/src/mutators/mod.rs +++ b/libafl/src/mutators/mod.rs @@ -32,7 +32,7 @@ use crate::{ /// The result of a mutation. /// If the mutation got skipped, the target /// will not be executed with the returned input. -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum MutationResult { /// The [`Mutator`] mutated this `Input`. Mutated, diff --git a/libafl/src/schedulers/powersched.rs b/libafl/src/schedulers/powersched.rs index b39a9d2b34..f9e6055df2 100644 --- a/libafl/src/schedulers/powersched.rs +++ b/libafl/src/schedulers/powersched.rs @@ -130,7 +130,7 @@ impl SchedulerMetadata { /// The power schedule to use #[allow(missing_docs)] -#[derive(Serialize, Deserialize, Clone, Copy, Debug, PartialEq)] +#[derive(Serialize, Deserialize, Clone, Copy, Debug, PartialEq, Eq)] pub enum PowerSchedule { EXPLORE, EXPLOIT, diff --git a/libafl_frida/src/asan/asan_rt.rs b/libafl_frida/src/asan/asan_rt.rs index 402ecf229c..9227a58946 100644 --- a/libafl_frida/src/asan/asan_rt.rs +++ b/libafl_frida/src/asan/asan_rt.rs @@ -1993,7 +1993,7 @@ impl AsanRuntime { ; self_addr: ; .qword self as *mut _ as *mut c_void as i64 ; self_regs_addr: - ; .qword &mut self.regs as *mut _ as *mut c_void as i64 + ; .qword addr_of_mut!(self.regs) as i64 ; trap_func: ; .qword AsanRuntime::handle_trap as *mut c_void as i64 ; register_frame_func: @@ -2565,7 +2565,7 @@ impl AsanRuntime { if displacement < 0 { if displacement > -4096 { #[allow(clippy::cast_sign_loss)] - let displacement = displacement.abs() as u32; + let displacement = displacement.unsigned_abs(); // Subtract the displacement into x0 writer.put_sub_reg_reg_imm( Aarch64Register::X0, @@ -2574,7 +2574,7 @@ impl AsanRuntime { ); } else { #[allow(clippy::cast_sign_loss)] - let displacement = displacement.abs() as u32; + let displacement = displacement.unsigned_abs(); let displacement_hi = displacement / 4096; let displacement_lo = displacement % 4096; writer.put_bytes(&(0xd1400000u32 | (displacement_hi << 10)).to_le_bytes()); diff --git a/libafl_frida/src/cmplog_rt.rs b/libafl_frida/src/cmplog_rt.rs index 0d3ca3666f..d68d4a3e8d 100644 --- a/libafl_frida/src/cmplog_rt.rs +++ b/libafl_frida/src/cmplog_rt.rs @@ -1,5 +1,5 @@ //! Functionality for [`frida`](https://frida.re)-based binary-only `CmpLog`. -//! With it, a fuzzer can collect feedback about each compare that happenned in the target +//! With it, a fuzzer can collect feedback about each compare that happened in the target //! This allows the fuzzer to potentially solve the compares, if a compare value is directly //! related to the input. //! Read the [`RedQueen`](https://www.ndss-symposium.org/ndss-paper/redqueen-fuzzing-with-input-to-state-correspondence/) paper for the general concepts. @@ -29,7 +29,7 @@ use frida_gum::{ use crate::utils::{instruction_width, writer_register}; #[cfg(all(feature = "cmplog", target_arch = "aarch64"))] -/// Speciial CmpLog Cases for `aarch64` +/// Speciial `CmpLog` Cases for `aarch64` #[derive(Debug)] pub enum SpecialCmpLogCase { /// Test bit and branch if zero @@ -44,8 +44,22 @@ use capstone::{ Capstone, Insn, }; +/// The [`frida_gum_sys::GUM_RED_ZONE_SIZE`] casted to [`i32`] +/// +/// # Panic +/// In debug mode, will panic on wraparound (which should never happen in practice) +#[cfg(all(feature = "cmplog", target_arch = "aarch64"))] +#[allow(clippy::cast_possible_wrap)] +fn gum_red_zone_size_i32() -> i32 { + debug_assert!( + i32::try_from(frida_gum_sys::GUM_RED_ZONE_SIZE).is_ok(), + "GUM_RED_ZONE_SIZE is bigger than i32::max" + ); + frida_gum_sys::GUM_RED_ZONE_SIZE as i32 +} + /// The type of an operand loggged during `CmpLog` -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] #[cfg(all(feature = "cmplog", target_arch = "aarch64"))] pub enum CmplogOperandType { /// A Register @@ -249,15 +263,16 @@ impl CmpLogRuntime { self.ops_handle_tbnz_masking.as_ref().unwrap() } - /// Emit the instrumentation code which is responsible for opernads value extraction and cmplog map population + /// Emit the instrumentation code which is responsible for operands value extraction and cmplog map population #[cfg(all(feature = "cmplog", target_arch = "aarch64"))] + #[allow(clippy::too_many_lines)] #[inline] pub fn emit_comparison_handling( &self, _address: u64, output: &StalkerOutput, - op1: CmplogOperandType, - op2: CmplogOperandType, + op1: &CmplogOperandType, + op2: &CmplogOperandType, special_case: Option, ) { let writer = output.writer(); @@ -267,17 +282,17 @@ impl CmpLogRuntime { Aarch64Register::X0, Aarch64Register::X1, Aarch64Register::Sp, - -(16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i32) as i64, + i64::from(-(16 + gum_red_zone_size_i32())), IndexMode::PreAdjust, ); // make sure operand1 value is saved into x0 match op1 { CmplogOperandType::Imm(value) | CmplogOperandType::Cimm(value) => { - writer.put_ldr_reg_u64(Aarch64Register::X0, value); + writer.put_ldr_reg_u64(Aarch64Register::X0, *value); } CmplogOperandType::Regid(reg) => { - let reg = writer_register(reg); + let reg = writer_register(*reg); match reg { Aarch64Register::X0 | Aarch64Register::W0 => {} Aarch64Register::X1 | Aarch64Register::W1 => { @@ -291,17 +306,17 @@ impl CmpLogRuntime { } } CmplogOperandType::Mem(basereg, indexreg, displacement, _width) => { - let basereg = writer_register(basereg); - let indexreg = if indexreg.0 != 0 { - Some(writer_register(indexreg)) - } else { + let basereg = writer_register(*basereg); + let indexreg = if indexreg.0 == 0 { None + } else { + Some(writer_register(*indexreg)) }; // calculate base+index+displacment into x0 let displacement = displacement + if basereg == Aarch64Register::Sp { - 16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i32 + 16 + gum_red_zone_size_i32() } else { 0 }; @@ -324,7 +339,10 @@ impl CmpLogRuntime { } } + debug_assert!(displacement >= 0); + //add displacement + #[allow(clippy::cast_sign_loss)] writer.put_add_reg_reg_imm( Aarch64Register::X0, Aarch64Register::X0, @@ -339,21 +357,21 @@ impl CmpLogRuntime { // make sure operand2 value is saved into x1 match op2 { CmplogOperandType::Imm(value) | CmplogOperandType::Cimm(value) => { - writer.put_ldr_reg_u64(Aarch64Register::X1, value); + writer.put_ldr_reg_u64(Aarch64Register::X1, *value); match special_case { Some(inst) => match inst { SpecialCmpLogCase::Tbz => { - writer.put_bytes(&self.ops_handle_tbz_masking()); + writer.put_bytes(self.ops_handle_tbz_masking()); } SpecialCmpLogCase::Tbnz => { - writer.put_bytes(&self.ops_handle_tbnz_masking()); + writer.put_bytes(self.ops_handle_tbnz_masking()); } }, None => (), } } CmplogOperandType::Regid(reg) => { - let reg = writer_register(reg); + let reg = writer_register(*reg); match reg { Aarch64Register::X1 | Aarch64Register::W1 => {} Aarch64Register::X0 | Aarch64Register::W0 => { @@ -371,17 +389,17 @@ impl CmpLogRuntime { } } CmplogOperandType::Mem(basereg, indexreg, displacement, _width) => { - let basereg = writer_register(basereg); - let indexreg = if indexreg.0 != 0 { - Some(writer_register(indexreg)) - } else { + let basereg = writer_register(*basereg); + let indexreg = if indexreg.0 == 0 { None + } else { + Some(writer_register(*indexreg)) }; - // calculate base+index+displacment into x1 + // calculate base+index+displacement into x1 let displacement = displacement + if basereg == Aarch64Register::Sp { - 16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i32 + 16 + gum_red_zone_size_i32() } else { 0 }; @@ -392,7 +410,7 @@ impl CmpLogRuntime { Aarch64Register::X0 | Aarch64Register::W0 => { match basereg { Aarch64Register::X1 | Aarch64Register::W1 => { - // x0 is overwrittern indexreg by op1 value. + // x0 is overwritten indexreg by op1 value. // x1 is basereg // Preserve x2, x3: @@ -400,7 +418,7 @@ impl CmpLogRuntime { Aarch64Register::X2, Aarch64Register::X3, Aarch64Register::Sp, - -(16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i32) as i64, + i64::from(-(16 + gum_red_zone_size_i32())), IndexMode::PreAdjust, ); @@ -422,7 +440,7 @@ impl CmpLogRuntime { Aarch64Register::X2, Aarch64Register::X3, Aarch64Register::Sp, - 16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i64, + 16 + i64::from(frida_gum_sys::GUM_RED_ZONE_SIZE), IndexMode::PostAdjust, )); } @@ -448,7 +466,7 @@ impl CmpLogRuntime { Aarch64Register::X1 | Aarch64Register::W1 => { match basereg { Aarch64Register::X0 | Aarch64Register::W0 => { - // x0 is overwrittern basereg by op1 value. + // x0 is overwritten basereg by op1 value. // x1 is indexreg // Preserve x2, x3: @@ -456,7 +474,7 @@ impl CmpLogRuntime { Aarch64Register::X2, Aarch64Register::X3, Aarch64Register::Sp, - -(16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i32) as i64, + i64::from(-(16 + gum_red_zone_size_i32())), IndexMode::PreAdjust, ); @@ -478,7 +496,7 @@ impl CmpLogRuntime { Aarch64Register::X2, Aarch64Register::X3, Aarch64Register::Sp, - 16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i64, + 16 + i64::from(frida_gum_sys::GUM_RED_ZONE_SIZE), IndexMode::PostAdjust, )); } @@ -494,7 +512,7 @@ impl CmpLogRuntime { _ => { match basereg { Aarch64Register::X0 | Aarch64Register::W0 => { - //basereg is overwrittern by op1 value + //basereg is overwritten by op1 value //index reg is not x0 nor x1 //reload basereg to x1 @@ -544,6 +562,7 @@ impl CmpLogRuntime { } // add displacement + #[allow(clippy::cast_sign_loss)] writer.put_add_reg_reg_imm( Aarch64Register::X1, Aarch64Register::X1, @@ -555,39 +574,37 @@ impl CmpLogRuntime { } //call cmplog runtime to populate the values map - writer.put_bytes(&self.ops_save_register_and_blr_to_populate()); + writer.put_bytes(self.ops_save_register_and_blr_to_populate()); // Restore x0, x1 assert!(writer.put_ldp_reg_reg_reg_offset( Aarch64Register::X0, Aarch64Register::X1, Aarch64Register::Sp, - 16 + frida_gum_sys::GUM_RED_ZONE_SIZE as i64, + 16 + i64::from(frida_gum_sys::GUM_RED_ZONE_SIZE), IndexMode::PostAdjust, )); } #[cfg(all(feature = "cmplog", target_arch = "aarch64"))] + #[allow(clippy::similar_names)] #[inline] /// Check if the current instruction is cmplog relevant one(any opcode which sets the flags) + #[must_use] pub fn cmplog_is_interesting_instruction( - &self, capstone: &Capstone, _address: u64, instr: &Insn, - ) -> Result< - ( - CmplogOperandType, - CmplogOperandType, - Option, - ), - (), - > { - // We only care for compare instrunctions - aka instructions which set the flags + ) -> Option<( + CmplogOperandType, + CmplogOperandType, + Option, + )> { + // We only care for compare instructions - aka instructions which set the flags match instr.mnemonic().unwrap() { "cmp" | "ands" | "subs" | "adds" | "negs" | "ngcs" | "sbcs" | "bics" | "cbz" | "cbnz" | "tbz" | "tbnz" | "adcs" => (), - _ => return Err(()), + _ => return None, } let mut operands = capstone .insn_detail(instr) @@ -601,7 +618,7 @@ impl CmpLogRuntime { ] .contains(&instr.mnemonic().unwrap()); if operands.len() != 2 && !special_case { - return Err(()); + return None; } // handle special opcodes case which have 3 operands, but the 1st(dest) is not important to us @@ -611,11 +628,10 @@ impl CmpLogRuntime { } // cbz marked as special since there is only 1 operand - let special_case = match instr.mnemonic().unwrap() { - "cbz" | "cbnz" => true, - _ => false, - }; + #[allow(clippy::cast_sign_loss)] + let special_case = matches!(instr.mnemonic().unwrap(), "cbz" | "cbnz"); + #[allow(clippy::cast_sign_loss, clippy::similar_names)] let operand1 = if let Arm64Operand(arm64operand) = operands.first().unwrap() { match arm64operand.op_type { Arm64OperandType::Reg(regid) => Some(CmplogOperandType::Regid(regid)), @@ -627,33 +643,30 @@ impl CmpLogRuntime { instruction_width(instr, &operands), )), Arm64OperandType::Cimm(val) => Some(CmplogOperandType::Cimm(val as u64)), - _ => return Err(()), + _ => return None, } } else { None }; #[allow(clippy::cast_sign_loss)] - let operand2 = match special_case { - true => Some(CmplogOperandType::Imm(0)), - false => { - if let Arm64Operand(arm64operand2) = &operands[1] { - match arm64operand2.op_type { - Arm64OperandType::Reg(regid) => Some(CmplogOperandType::Regid(regid)), - Arm64OperandType::Imm(val) => Some(CmplogOperandType::Imm(val as u64)), - Arm64OperandType::Mem(opmem) => Some(CmplogOperandType::Mem( - opmem.base(), - opmem.index(), - opmem.disp(), - instruction_width(instr, &operands), - )), - Arm64OperandType::Cimm(val) => Some(CmplogOperandType::Cimm(val as u64)), - _ => return Err(()), - } - } else { - None - } + let operand2 = if special_case { + Some(CmplogOperandType::Imm(0)) + } else if let Arm64Operand(arm64operand2) = &operands[1] { + match arm64operand2.op_type { + Arm64OperandType::Reg(regid) => Some(CmplogOperandType::Regid(regid)), + Arm64OperandType::Imm(val) => Some(CmplogOperandType::Imm(val as u64)), + Arm64OperandType::Mem(opmem) => Some(CmplogOperandType::Mem( + opmem.base(), + opmem.index(), + opmem.disp(), + instruction_width(instr, &operands), + )), + Arm64OperandType::Cimm(val) => Some(CmplogOperandType::Cimm(val as u64)), + _ => return None, } + } else { + None }; // tbz will need to have special handling at emit time(masking operand1 value with operand2) @@ -663,15 +676,16 @@ impl CmpLogRuntime { _ => None, }; - if operand1.is_some() && operand2.is_some() { - Ok((operand1.unwrap(), operand2.unwrap(), special_case)) + if let Some(op1) = operand1 { + operand2.map(|op2| (op1, op2, special_case)) } else { - Err(()) + None } } } impl Default for CmpLogRuntime { + #[inline] fn default() -> Self { Self::new() } diff --git a/libafl_frida/src/coverage_rt.rs b/libafl_frida/src/coverage_rt.rs index 68e67f7079..016c770572 100644 --- a/libafl_frida/src/coverage_rt.rs +++ b/libafl_frida/src/coverage_rt.rs @@ -108,7 +108,7 @@ impl CoverageRuntime { ; ldp x1, x2, [sp], #0x10 ; ret ;map_addr: - ;.qword &mut self.map as *mut _ as *mut c_void as i64 + ;.qword addr_of_mut!(self.map) as i64 ;previous_loc: ;.qword 0 ); diff --git a/libafl_frida/src/helper.rs b/libafl_frida/src/helper.rs index 2416315ca8..32c52183bc 100644 --- a/libafl_frida/src/helper.rs +++ b/libafl_frida/src/helper.rs @@ -324,15 +324,19 @@ where #[cfg(all(feature = "cmplog", target_arch = "aarch64"))] if let Some(rt) = helper.runtime::() { - if let Ok((op1, op2, special_case)) = rt - .cmplog_is_interesting_instruction(&helper.capstone, address, instr) + if let Some((op1, op2, special_case)) = + CmpLogRuntime::cmplog_is_interesting_instruction( + &helper.capstone, + address, + instr, + ) { //emit code that saves the relevant data in runtime(passes it to x0, x1) rt.emit_comparison_handling( address, &output, - op1, - op2, + &op1, + &op2, special_case, ); } diff --git a/libafl_qemu/src/emu.rs b/libafl_qemu/src/emu.rs index 7a34314911..8e0a2390ab 100644 --- a/libafl_qemu/src/emu.rs +++ b/libafl_qemu/src/emu.rs @@ -27,7 +27,7 @@ use pyo3::{prelude::*, PyIterProtocol}; pub const SKIP_EXEC_HOOK: u64 = u64::MAX; -#[derive(IntoPrimitive, TryFromPrimitive, Debug, Clone, Copy, EnumIter, PartialEq)] +#[derive(IntoPrimitive, TryFromPrimitive, Debug, Clone, Copy, EnumIter, PartialEq, Eq)] #[repr(i32)] pub enum MmapPerms { None = 0,