smite: extract Violation enum for target-bug findings#142
Conversation
| /// The target broke a protocol invariant. Surfaced to the scenario as a | ||
| /// failure; see [`Violation`] for the full catalog of target-bug findings. | ||
| #[error(transparent)] | ||
| Violation(#[from] Violation), |
There was a problem hiding this comment.
nit: I think Violation as a name is too generic. It doesn't say what was violated. Could rename to ProtocolViolation? I assume this is only meant for protocol violations, as per the documentation and current usage?
There was a problem hiding this comment.
See my comment below
| /// The target process died during or after processing the input. | ||
| #[error("target crashed")] | ||
| Crashed, | ||
|
|
||
| /// The target stopped responding to the post-input ping-pong sync. | ||
| #[error("target hung (ping timeout)")] | ||
| Hung, |
There was a problem hiding this comment.
Regarding my suggestion to rename to ProtocolViolation: these two aren't as clearly protocol violations as the others
There was a problem hiding this comment.
Violation is intentionally generic: it's the catalog of everything the fuzzer judges to be a target bug, not just protocol violations. A crash or a hang isn't a protocol violation, but it's still a violation of what a correct target should do, so ProtocolViolation would be too narrow for the enum as a whole. That's also why I kept Crashed/Hung in it. Note that the doc on ExecuteError::Violation does say "protocol invariant". That's accurate there, since the executor only ever produces the protocol-shaped variants; Crashed/Hung are raised directly by the scenarios and never go through ExecuteError.
There was a problem hiding this comment.
Thanks for the explanation. Makes sense to me!
erickcestari
force-pushed
the
extract-violation
branch
from
92a1bad to
7d2bf6f
Compare
Carve target-misbehavior findings out of ExecuteError into a dedicated Violation enum, and replace raw finding strings in scenarios with its variants. Centralizes the finding vocabulary so new oracles route through a single match arm and each message lives in one place.
erickcestari
force-pushed
the
extract-violation
branch
from
7d2bf6f to
8628e82
Compare
|
Thanks for the review @ekzyis! I've addressed most of your comments and added a comment about |
| /// The target process died during or after processing the input. | ||
| #[error("target crashed")] | ||
| Crashed, | ||
|
|
||
| /// The target stopped responding to the post-input ping-pong sync. | ||
| #[error("target hung (ping timeout)")] | ||
| Hung, |
There was a problem hiding this comment.
Thanks for the explanation. Makes sense to me!
2 participants
Carve target-misbehavior findings out of
ExecuteErrorinto a dedicatedViolationenum, and replace raw finding strings in scenarios with its variants. Centralizes the finding vocabulary so new oracles route through a single match arm and each message lives in one place.