smitebot: avoid afl-cmin filename overflow on direct corpus minimize - #190
smitebot: avoid afl-cmin filename overflow on direct corpus minimize#190Ashish-Kumar-Dash wants to merge 1 commit into
Conversation
b2591f4 to
e05db7a
Compare
afl-cmin -X copies each input to `<8hex>_<basename>`; smite-ir queue filenames embed the full mutation stack and already approach NAME_MAX (255), so the prefix overflows (ENAMETOOLONG) on a direct minimize of raw runner queues. Stage the queues through the existing merge dedup into a short-named temp corpus first, then run afl-cmin on that and clean it up. A user-supplied --input is passed through unchanged.
54490b2 to
fb3af5f
Compare
morehouse
left a comment
There was a problem hiding this comment.
I think we probably want to always do the merge step first, not just in the default-no-input case. A manually-specified a queue dir as input is going to have the same long-pathname issue. This probably also means we need the ability to specify multiple --input flags.
| /// Stages a campaign's runner queues into `stage_dir` with short, sequential | ||
| /// filenames, returning the directory to hand to `afl-cmin -i`. | ||
| /// | ||
| /// afl-cmin -X hardlinks each input to `<8hex>_<basename>` (batch mode); smite-ir |
There was a problem hiding this comment.
Is this true? I don't recall seeing such name formats; I thought it just preserved the existing filenames.
|
|
||
| let (total_in, total_out) = merge_states(std::slice::from_ref(state), stage_dir)?; | ||
| log::info!("staged {total_out} unique inputs (deduped from {total_in}) for minimization"); | ||
| Some(stage_dir.display().to_string()) |
There was a problem hiding this comment.
Can we return a PathBuf for simplicity?
| let mut cmd = Command::new(&afl_cmin); | ||
| cmd.arg("-i") | ||
| .arg(&input) | ||
| .arg("-o") | ||
| .arg(&output) | ||
| .arg("-X") | ||
| .arg(&state.sharedir); | ||
|
|
||
| let status = match cmd.status() { | ||
| let status = cmd.status(); | ||
|
|
||
| // Remove the staging corpus we created (whether or not afl-cmin succeeded); | ||
| // the minimized output in `output` is what the user keeps. | ||
| if args.input.is_none() | ||
| && let Err(e) = fs::remove_dir_all(&stage_dir) | ||
| { | ||
| log::warn!( | ||
| "failed to remove staging directory {}: {e}", | ||
| stage_dir.display() | ||
| ); | ||
| } | ||
|
|
||
| let status = match status { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| log::error!("failed to run {}: {e}", afl_cmin.display()); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Nit: I think it would be cleaner to add a run_afl_cmin helper function that does everything as a unit, and then we can remove the stage dir after that call.
| /// afl-cmin -X hardlinks each input to `<8hex>_<basename>` (batch mode); smite-ir | ||
| /// queue filenames already approach `NAME_MAX`, so the added prefix overflows | ||
| /// (`ENAMETOOLONG`). Reusing `merge`'s content dedup gives short names (and drops | ||
| /// duplicates, which afl-cmin would discard anyway). Returns `None` on failure, | ||
| /// logged at the site. |
There was a problem hiding this comment.
This description is quite wordy and difficult to understand.
| /// afl-cmin -X hardlinks each input to `<8hex>_<basename>` (batch mode); smite-ir | |
| /// queue filenames already approach `NAME_MAX`, so the added prefix overflows | |
| /// (`ENAMETOOLONG`). Reusing `merge`'s content dedup gives short names (and drops | |
| /// duplicates, which afl-cmin would discard anyway). Returns `None` on failure, | |
| /// logged at the site. | |
| /// AFL++ tends to produce long input names near `NAME_MAX`, and afl-cmin can then | |
| /// exceed the max length during minimization. Do an implicit merge step first so that | |
| /// inputs get a short name and afl-cmin is less likely to exceed the max path length. |
|
|
||
| - `<campaign-id>`: directory name under `~/.smitebot/runs` | ||
| - `-i, --input <input>`: input directory or glob pattern passed to `afl-cmin -i`; defaults to `<output_dir>/*/queue/` (all runner queues) | ||
| - `-i, --input <input>`: input directory passed to `afl-cmin -i`; by default the campaign's runner queues are staged into a short-named temp corpus first (afl-cmin's `<hash>_<name>` staging overflows `NAME_MAX` on long smite-ir queue filenames), so no `--input` is needed. A supplied `--input` is passed through unchanged |
There was a problem hiding this comment.
This seems like implementation details the user doesn't need to know.
| - `-i, --input <input>`: input directory passed to `afl-cmin -i`; by default the campaign's runner queues are staged into a short-named temp corpus first (afl-cmin's `<hash>_<name>` staging overflows `NAME_MAX` on long smite-ir queue filenames), so no `--input` is needed. A supplied `--input` is passed through unchanged | |
| - `-i, --input <input>`: input directory passed to `afl-cmin -i`; by default the campaign's runner queues are implicitly merged into a single directory that is passed to this flag |
| let input = match &args.input { | ||
| Some(i) => i.clone(), | ||
| None => match stage_queues(&state, &stage_dir) { | ||
| Some(dir) => dir, | ||
| None => return false, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Nit:
| let input = match &args.input { | |
| Some(i) => i.clone(), | |
| None => match stage_queues(&state, &stage_dir) { | |
| Some(dir) => dir, | |
| None => return false, | |
| }, | |
| }; | |
| let Some(input) = args.input.clone().or_else(|| stage_queues(&state, &stage_dir)) else { | |
| return false; | |
| } |
Follow-up to the corpus command #184 .
The default path now stages the campaign's queues through the same content-dedup as
mergeinto a short-named temp corpus, runs afl-cmin on that, and removes it afterward. A supplied--inputstays passthrough.