Panic on invalid programs and bad seed inputs

[morehouse]

Demonstrates how we can simplify our code using the approach described in #92 (comment).

Pros:

• simpler code
• faster code
• fails fast on mutator/generator bugs, bad seed corpus, or misconfigured AFL++ (e.g., IR fuzzer produces postcard-encoded programs with out-of-bounds ShutdownScriptVariant values, panicking the executor #82)

Cons:

• our custom mutator library is now required for IR fuzzing
• hand-created seeds or seeds from previous versions of the IR are likely to trigger panics instead of silently being skipped

Lightly tested with a 10 minute LDK fuzzing session. If we decide we want to do this, we'll need to test longer runs of all 4 targets.

[Chand-ra]

Our custom mutators are the only supported producers of IR programs and
are designed to always create valid programs. Thus validating
programs on every mutation iteration is useless work for all supported
use cases.

I'm not sure how future proof this is. I think this approach will force mutators to do two things:

1. Mutate a given program.
2. Verify that you never produce an invalid mutation.

IMO, mutators should be dumb and try to do only (1). I guess doing (2) is fine as well (and we already do it), as long as we're talking about structural validity, i.e. validity of the program graph, because the information required to enforce it is already provided to the mutators (in the form of the program itself).

But as discussed in #75, once we start implementing more advanced mutation schemes, this means we will have to maintain a rulebook of sorts for every new mutator that tries to modify the semantic structure of the program.

Subsequent commits will remove program validation entirely, instead
relying on the ProgramBuilder and executor panicking when programs are
invalid.

Okay, so the "rulebook" is supposed to reside in the executor. I don't feel very strongly about the executor panicking or not panicking, but I think it makes more sense for Program to dictate what a valid version of it looks like.

[NishantBansal2003]

I think with this we get slightly faster code, since all the validation paths are gone. Previously, we had to maintain the validity of IR programs for the custom mutator anyway, and the same still holds now. So the main thing we lose is the ability to use AFL++’s default mutators, which were not very helpful anyway (especially once we have more interesting mutators in smite).

I would lean towards the simpler and faster approach here, and be explicit that IR programs should use a custom mutator instead of wasting cycles on Skip. I think optimizations and performance improvements on the IR or mutator side are much more likely to uncover target bugs than relying on AFL++’s default mutators.

The tradeoff is that manually-created
seeds or even seeds from previous versions of Smite will cause panics
rather than being silently discarded.

Though right now, adding a new operation is not backward compatible anyway, so previously collected corpora would already become invalid. Because of that, I don’t think resuming old corpora after adding new operations would help much for further fuzzing, so it is probably fine for them to be paniced/discarded completely.

[ekzyis]

our custom mutator library is now required for IR fuzzing

nit: Could add a check that AFL_CUSTOM_MUTATOR_LIBRARY (and AFL_CUSTOM_MUTATOR_ONLY) is set, and if not, at least print a warning?

[ekzyis]

As written, the message suggests that we're expecting the mutator to produce an invalid private key (but it didn't), while we're actually expecting that it didn't (but it did). So similar to how expect() is used in other places, I suggest this:

Suggested change

	.expect("mutator produced an invalid private key");
	.expect("valid private key");

[morehouse]

I'm not sure how future proof this is. I think this approach will force mutators to do two things:

1. Mutate a given program.

2. Verify that you never produce an invalid mutation.

IMO, mutators should be dumb and try to do only (1). I guess doing (2) is fine as well (and we already do it), as long as we're talking about structural validity, i.e. validity of the program graph, because the information required to enforce it is already provided to the mutators (in the form of the program itself).

But as discussed in #75, once we start implementing more advanced mutation schemes, this means we will have to maintain a rulebook of sorts for every new mutator that tries to modify the semantic structure of the program.

Subsequent commits will remove program validation entirely, instead
relying on the ProgramBuilder and executor panicking when programs are
invalid.

Okay, so the "rulebook" is supposed to reside in the executor. I don't feel very strongly about the executor panicking or not panicking, but I think it makes more sense for Program to dictate what a valid version of it looks like.

I think we are in agreement here.

I'm using the term valid in the structural sense -- i.e. a valid program is runnable by the executor. I want mutators to always produce runnable programs but otherwise be ignorant of the higher-level LN protocol semantics (e.g., message sequencing). I think generators are better suited to encapsulate the semantics, and mutators should be free to break them.

The main changes proposed in this PR are really:

1. Remove Program::validate since it is really just an approximation of the validation done during execute.
2. Make execute panic on any invalidly-structured program instead of returning an error.

[Chand-ra]

I'm using the term valid in the structural sense -- i.e. a valid program is runnable by the executor. I want mutators to always produce runnable programs but otherwise be ignorant of the higher-level LN protocol semantics (e.g., message sequencing). I think generators are better suited to encapsulate the semantics, and mutators should be free to break them.

Right.

The main changes proposed in this PR are really:

1. Remove Program::validate since it is really just an approximation of the validation done during execute.

Okay, this is where I think the friction arises from. Both Program::validate() and execute() protect against structurally invalid programs for now, but in the future we will need to add validation rules for protocol semantics of LN as well, and Program::validate() is much better equipped to handle that than execute().

2. Make execute panic on any invalidly-structured program instead of returning an error.

Yeah, I don't feel too strongly about this, either way is fine.