Fix racy file operations in libfuzzer_libpng_launcher & Sort outputs by ClientId in OnDiskTomlMonitor (#3847)

* Fix racey file operations in libfuzzer_libpng_launcher fuzzer

* ClientStatsManager: Keep sorted list of client ids

Author: Ollrogge

crates/libafl/src/monitors/disk.rs

@@ -1,6 +1,6 @@
 //! Monitors that log to disk using different formats like `JSON` and `TOML`.
 
-use alloc::{string::String, vec::Vec};
+use alloc::string::String;
 use core::time::Duration;
 use std::{
     fs::{File, OpenOptions},
@@ -61,19 +61,14 @@ exec_sec = {}
             )
             .expect("Failed to write to the Toml file");
 
-            let all_clients: Vec<ClientId> = client_stats_manager
-                .client_stats()
-                .keys()
-                .copied()
-                .collect();
-
-            for client_id in &all_clients {
+            for idx in 0..client_stats_manager.client_ids_sorted().len() {
+                let client_id = client_stats_manager.client_ids_sorted()[idx];
                 let exec_sec = client_stats_manager
-                    .update_client_stats_for(*client_id, |client_stat| {
+                    .update_client_stats_for(client_id, |client_stat| {
                         client_stat.execs_per_sec(cur_time)
                     })?;
 
-                let client = client_stats_manager.client_stats_for(*client_id)?;
+                let client = client_stats_manager.client_stats_for(client_id)?;
 
                 write!(
                     &mut file,

crates/libafl/src/monitors/stats/manager.rs

@@ -1,6 +1,6 @@
 //! Client statistics manager
 
-use alloc::{borrow::Cow, string::String};
+use alloc::{borrow::Cow, string::String, vec::Vec};
 use core::time::Duration;
 
 use hashbrown::HashMap;
@@ -19,6 +19,7 @@ use super::{
 /// Manager of all client's statistics
 #[derive(Debug)]
 pub struct ClientStatsManager {
+    sorted_client_ids: Vec<ClientId>,
     client_stats: HashMap<ClientId, ClientStats>,
     /// Aggregated user stats value.
     ///
@@ -36,6 +37,7 @@ impl ClientStatsManager {
     #[must_use]
     pub fn new() -> Self {
         Self {
+            sorted_client_ids: Vec::new(),
             client_stats: HashMap::new(),
             cached_aggregated_user_stats: HashMap::new(),
             cached_global_stats: None,
@@ -49,6 +51,12 @@ impl ClientStatsManager {
         &self.client_stats
     }
 
+    /// Get sorted client ids
+    #[must_use]
+    pub fn client_ids_sorted(&self) -> &[ClientId] {
+        &self.sorted_client_ids
+    }
+
     /// Get client with `client_id`
     pub fn get(&self, client_id: ClientId) -> Result<&ClientStats, Error> {
         self.client_stats
@@ -68,6 +76,16 @@ impl ClientStatsManager {
             };
             self.client_stats.insert(client_id, stats);
             self.cached_global_stats = None;
+
+            if self
+                .sorted_client_ids
+                .last()
+                .map_or(true, |last| *last < client_id)
+            {
+                self.sorted_client_ids.push(client_id);
+            } else if let Err(idx) = self.sorted_client_ids.binary_search(&client_id) {
+                self.sorted_client_ids.insert(idx, client_id);
+            }
         }
 
         self.update_client_stats_for(client_id, |new_stat| {
@@ -110,6 +128,8 @@ impl ClientStatsManager {
     /// This will clear global stats cache.
     pub fn update_all_client_stats(&mut self, new_client_stats: HashMap<ClientId, ClientStats>) {
         self.client_stats = new_client_stats;
+        self.sorted_client_ids = self.client_stats.keys().copied().collect();
+        self.sorted_client_ids.sort_unstable();
         self.cached_global_stats = None;
     }
 

crates/libafl/src/stages/afl_stats.rs

@@ -538,13 +538,10 @@ where
     /// Writes a stats file, if a `stats_file_path` is set.
     fn maybe_write_fuzzer_stats(&self, stats: &AflFuzzerStats) -> Result<(), Error> {
         if let Some(stats_file_path) = &self.stats_file_path {
-            let tmp_file = stats_file_path
-                .parent()
-                .expect("fuzzer_stats file must have a parent!")
-                .join(".fuzzer_stats_tmp");
+            let tmp_file = stats_file_path.with_extension("tmp");
+
             std::fs::write(&tmp_file, stats.to_string())?;
-            _ = std::fs::copy(&tmp_file, stats_file_path)?;
-            std::fs::remove_file(tmp_file)?;
+            _ = std::fs::rename(&tmp_file, stats_file_path)?;
         }
         Ok(())
     }

fuzzers/inprocess/libfuzzer_libpng_launcher/src/lib.rs

@@ -344,12 +344,15 @@ pub extern "C" fn libafl_main() {
             ]));
         }
 
+        // filename needs to be unique per client
+        let stats_file = format!("fuzzer_stats{}", client_description.id());
+
         // Setup a basic mutator with a mutational stage
         let mutator = HavocScheduledMutator::new(havoc_mutations().merge(tokens_mutations()));
         let calibration = CalibrationStage::new(&map_feedback);
         let afl_stats = AflStatsStage::builder()
             .map_feedback(&map_feedback)
-            .stats_file(opt.output.join("fuzzer_stats"))
+            .stats_file(opt.output.join(stats_file))
             .report_interval(Duration::from_millis(1000))
             .core_id(core_id)
             .banner("libfuzzer_libpng".into())