smite-ir: Add InstructionDeleteMutator

[Chand-ra]

Add an InstructionDelete mutator for Smite IR. Mutates a given program by deleting a randomly selected instruction.

Deletion of an instruction can happen in one of two ways:
(i) Replacing it with a Operation::Nop (50% chance).
(ii) Removing it from the instructions list and reindexing the subsequent instructions (50% chance).

[erickcestari]

Deletion of an instruction can happen in one of two ways: (i) Replacing it with a Operation::Nop (50% chance). (ii) Removing it from the instructions list and reindexing the subsequent instructions (50% chance).

Are there any advantages to having two ways to delete an operation? If not, I would prefer to use only Operation::Nop for simplicity and performance. Also, noping a random instruction could invalidate program correctness (e.g., if any operation references a NOP instruction, it will cause a panic). So we can only Nop instructions with no consumers.

[Chand-ra]

Are there any advantages to having two ways to delete an operation? If not, I would prefer to use only Operation::Nop for simplicity and performance.

If we only keep the Nop mutation path and the fuzzer triggers this mutator multiple times, we run into the risk of replacing most of the program with Nop, which will severely affect subsequent mutations. Index renumbering allows us to actually shrink the program and occasionally get rid of these inserted Nops as well.

If we want to only keep one mutation scheme, I say we should go with index renumbering.

Also, noping a random instruction could invalidate program correctness (e.g., if any operation references a NOP instruction, it will cause a panic). So we can only Nop instructions with no consumers.

Yes, we handle that in this part of the code.

We first check if any operation uses our deleted instruction as input. If yes, we search for a previously defined instruction with the same type as the deleted one (returns false if no such instruction exists) and then replace instances of our deleted instruction with the found replacement.

[morehouse]

Perhaps there's a middle path -- we could have our mutators insert Nop, and have our nop-minimizer (see #54) rewrite the minimized program without nops? That would allow mutators to do the simple, quick thing, while minimizers periodically clean up the nops during the afl-trim phase. WDYT?

[erickcestari]

Perhaps there's a middle path -- we could have our mutators insert Nop, and have our nop-minimizer (see #54) rewrite the minimized program without nops? That would allow mutators to do the simple, quick thing, while minimizers periodically clean up the nops during the afl-trim phase. WDYT?

Sounds good to me.

[Chand-ra]

Perhaps there's a middle path -- we could have our mutators insert Nop, and have our nop-minimizer (see #54) rewrite the minimized program without nops? That would allow mutators to do the simple, quick thing, while minimizers periodically clean up the nops during the afl-trim phase. WDYT?

If I'm not wrong, the nop-minimizer replaces unused instructions with NOP. So to achieve your suggestion, we would need to modify the minimizer to remove the added NOPs, or write a separate minimizer that cleans up these NOPs (although I guess nop-minimizer should do this anyway).

Looks to me like we're deferring complexity in the mutator to other parts of the codebase. Why not make the mutator never introduce NOPs in the first place?

Unless of course, if it's better for nop-minimizer to always get rid of dead instructions instead of nopping them, in which case I agree that this would be a good idea.

[erickcestari]

After reconsidering, it likely makes sense to remove NOP entirely. Its only real justification would be performance, but the fuzzer mutator is not the bottleneck. The executor, performing cryptographic operations and I/O over a TCP, handshake-bound connection, is orders of magnitude more expensive than any overhead introduced here. I also think this PR could drop the 50% chance of replacing with NOP and keep only the remove-and-reindex path.

I'll rewrite my trimmer/minimizer PR to also do remove-and-reindex, but more aggressively, for all instructions that have no consumers, while maintaining the same coverage. Subsequent mutations should then be more efficient, since they won't waste exec budget mutating dead IR.

What do you think @morehouse @Chand-ra ?

[Chand-ra]

After reconsidering, it likely makes sense to remove NOP entirely. Its only real justification would be performance, but the fuzzer mutator is not the bottleneck. The executor, performing cryptographic operations and I/O over a TCP, handshake-bound connection, is orders of magnitude more expensive than any overhead introduced here. I also think this PR could drop the 50% chance of replacing with NOP and keep only the remove-and-reindex path.

I'll rewrite my trimmer/minimizer PR to also do remove-and-reindex, but more aggressively, for all instructions that have no consumers, while maintaining the same coverage. Subsequent mutations should then be more efficient, since they won't waste exec budget mutating dead IR.

What do you think @morehouse @Chand-ra ?

Sounds good to me. But as Matt mentioned earlier, maybe inserting NOPs here and deferring cleanup to the minimizer is a better idea? That way we can get rid of some complexity in this mutator AND avoid duplicating reindexing logic.

[erickcestari]

Sounds good to me. But as Matt mentioned earlier, maybe inserting NOPs here and deferring cleanup to the minimizer is a better idea? That way we can get rid of some complexity in this mutator AND avoid duplicating reindexing logic.

Either way is fine to me. If you both prefer going with NOPs I'll update my PR for it.

[Chand-ra]

Sounds good to me. But as Matt mentioned earlier, maybe inserting NOPs here and deferring cleanup to the minimizer is a better idea? That way we can get rid of some complexity in this mutator AND avoid duplicating reindexing logic.

Either way is fine to me. If you both prefer going with NOPs I'll update my PR for it.

I'll let Matt take the call on this, seeing how afl_custom_trim isn't always called after a custom mutate, so we could still run into the risk of nopping the entire program. Either way, we need to keep the following in mind when choosing one of the mutation schemes:

• Only nopping runs into the risk of replacing most of the program with NOP, which may severely affect subsequent mutations, especially long runs. It is however lower on complexity (in terms of both execution and implementation) and the too-much-nopping problem may be mitigated by the newly introduced dead_code custom trimmer.

• Reindexing has higher complexity (again, in terms of both execution and implementation) but it never introduces dead code into the program, which is a more efficient use of fuzzing cycles. However, we duplicate the re-indexing logic implemented in the dead_code trimmer.

I'm personally leaning towards reindexing because:

1. I'm unsure about the frequency of custom trimming after each custom mutation, which is the only way nopping makes sense.
2. As Erick noted, the mutator's time complexity is nothing compared to other phases of fuzzing.
3. The implementation complexity of either of these paths is minuscule compared to the mutator's healing phase, which we always need to perform.
4. Duplicating a bit of healing logic doesn't sound like the end of world to me.

[morehouse]

Let's ditch Nop entirely and rewrite instead.

[Chand-ra]

Rewrote the mutator and accompanying tests without the NOP path and rebased on top of the latest master to resolve a merge conflict.

[morehouse]

The same concern described in #61 (review) applies here: if we delete a SendMessage that precedes a Recv* operation, we may force a 5s timeout when executing the input.

I think we need a good solution to this issue before we can merge this mutator.

[morehouse]

It would be good to also test deletion of a void-output operation (i.e. SendMessage).

[Chand-ra]

I cannot say I see the merit in it, the only "interesting" thing that happens (from the mutator's perspective) on deleting a void-output operation is index-shifting of downstream instructions, which is tested here anyway. Maybe I'm missing something?

[Chand-ra]

Haven't squashed the changes yet because we won't merge this without solving the 'implicit dependence' issue anyway, and it should ease the reviewing.