Skip to content

refactor(dpi/stun): drop unreachable!() arm in class decode#289

Merged
domcyrus merged 1 commit into
domcyrus:mainfrom
0xghost42:refactor/dpi-stun-unreachable
May 19, 2026
Merged

refactor(dpi/stun): drop unreachable!() arm in class decode#289
domcyrus merged 1 commit into
domcyrus:mainfrom
0xghost42:refactor/dpi-stun-unreachable

Conversation

@0xghost42
Copy link
Copy Markdown
Contributor

Summary

Same shape as #279 for SNMP: collapse a match whose catch-all is provably unreachable into one that no longer needs it.

analyze_stun decodes the message class from two bits:

let c0 = (msg_type >> 4) & 0x1;
let c1 = (msg_type >> 8) & 0x1;
let class_bits = (c1 << 1) | c0;

Each of c0 / c1 is masked to a single bit, so class_bits is bounded to 0..=3 by construction. The match below already enumerated all four values and carried a _ => unreachable!() arm that the compiler could not prove away.

This PR:

  • Tightens c0 / c1 from u16 to u8, keeping the bit arithmetic narrow.
  • Replaces the unreachable!() with the 0b11 case (ErrorResponse), with a short comment recording the bound that justifies the change.
  • Adds test_class_bits_exhaustive_for_unknown_method that pairs every class with a non-Binding method (0x0003) so a future change to method handling can't silently make class decode method-dependent — the four existing per-class tests all use method=Binding and would still pass.

No behavior change in the success path; the _ arm previously panicked and now returns ErrorResponse, which is what unreachable!() was implicitly claiming would never be needed.

Verification

  • cargo test --lib: 360 passed, 0 failed (15 in network::dpi::stun, incl. the new test).
  • cargo clippy --all-targets -- -D warnings: clean.
  • cargo fmt --check: clean.

Notes

Reviewed RFC 5389 §6 (message type encoding) to confirm the four class encodings and the meaning of the M / C bits.

class_bits is computed as ((c1 << 1) | c0) where both c0 and c1 are
masked to a single bit, so its value is bounded to 0..=3 by
construction. With that invariant, the 0b11 arm covers the only
remaining value and the `_ => unreachable!()` catch-all carries no
information for either the reader or the compiler.

Replace the catch-all with the 0b11 case (ErrorResponse) and tighten
the c0/c1 types from u16 to u8 to keep the bit-arithmetic narrow.

Add a regression test that pairs every class with a non-Binding
method (0x0003) so future changes to method handling can't mask a
class-bit regression. The four existing per-class tests all use
method=Binding and would still pass even if the class decode silently
became dependent on method recognition.
@domcyrus
Copy link
Copy Markdown
Owner

@0xghost42 thanks a lot! This also looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants