Skip to content

fix: preserve supported questions when unknown types are present - #508

Open
Twister915 wants to merge 1 commit into
keepsimple1:mainfrom
Twister915:fix/parse-mixed-questions
Open

Twister915 wants to merge 1 commit into
keepsimple1:mainfrom
Twister915:fix/parse-mixed-questions

Conversation

@Twister915

@Twister915 Twister915 commented Sep 15, 2026

Copy link
Copy Markdown

Hello! I am submitting three fixes to bugs I encountered while developing a tvOS app for myself.

The app failed to discover my Rust service once again, and the root cause was determined to be an issue with dealing with an "unsupported question type" which my app was emitting. This patch allows you to process other requests which are supported.

Reading the patch... I realize this just turns an error into a silent failure. I could argue that this may be OK behavior, but only within the context of my application. I imagine other applications may want to handle this sort of partial failure differently. I am happy to discuss other possible designs and implement whatever we think makes the most sense. This, however, is an improvement over total failure as well, I'd say.

Here is the PR description that my agent wrote (gpt-6 astra):


A well-formed DNS packet containing one unsupported question type currently fails parsing as a whole. For example, an HTTPS (type 65) question bundled with AAAA and A causes the valid address questions to be discarded too.

Once the question's name, type, and class have been consumed and validated, skip an unsupported type and continue parsing the packet. Malformed or truncated questions still fail validation. This also preserves resource records after the question section.

The wire-format regressions cover types 64, 65, and a future type (65400) at the beginning, middle, and end of a mixed question section, followed by a known answer. They also cover an unknown-only question and truncated question fields. The supported-question checks remain valid if type 64/65 support is added separately; this PR does not need new enum variants.

Validation

  • macOS, nightly Rust: both targeted parser regressions pass. Restoring the original parser makes the mixed-question regression fail.
  • cargo fmt --check, async-only build, and all-feature documentation with warnings denied pass.
  • All-feature library suite: 56 passed; test_hostname_resolution_address_removed and test_interface_flip failed. The first also fails on untouched upstream 75d1941; the second passes when run alone on both this branch and the untouched base.
  • Strict nightly Clippy reports existing upstream diagnostics; comparison against the untouched base found no new diagnostics from this change.

This is one independent commit on current upstream main; no other PR is required.

@Twister915

Twister915 commented Sep 15, 2026

Copy link
Copy Markdown
Author

I had my agent spit out some options for this. I'm using this comment as a scratchpad to hold some design options / ideas.

This particular suggestion seems... a little heavy handed, adding a new envelope struct to contain the diagnostics.

Let me know your thoughts about the tradeoff between keeping track of these diagnostics vs tracing silent failure like I proposed originally. I feel like the current behavior, total failure, is 100% correct but at the cost of being a little impractical to use.

Below this line is AI


After looking more closely at the parser and public API, I think the concern about silent recovery is fair. Here is a possible direction for revising this.

Separate unsupported questions from malformed packets

A well-formed question with an unfamiliar numeric QTYPE is still parseable. We can decode its name, type, and class even if the responder cannot answer it. I would prefer to retain that information internally and let the responder handle the supported questions, while reporting which ones it could not handle. Invalid names, truncated fields, or other cases where parsing cannot safely continue should remain errors.

Retaining unknown questions also preserves the original question section for legacy-unicast replies. Simply skipping them loses that information. I would use a private raw-question representation or private Known(RRType) / Unknown(u16) wrapper, rather than add a payload-bearing variant to the public RRType: existing code casts that enum with as u16, so that apparently additive change would break callers.

Return structured recovery information internally

Conceptually, the internal parser could return:

Result<ParsedPacket, ParseError>

// Sketch, not a proposed public API:
struct ParsedPacket {
    message: DnsIncoming,
    diagnostics: Vec<ParseDiagnostic>,
}

For example, an UnsupportedQuestion diagnostic could contain the question index and original numeric type/class. Ok would mean the packet was safely decoded, possibly with unsupported content; Err would mean it cannot safely be processed. The daemon would process the usable content and separately expose the diagnostics. Unsupported content alone must not imply that a record does not exist or justify an NSEC response.

DnsIncoming lives in the private dns_parser module, so this does not require changing the public Result alias or the signatures of register, browse, etc. Those methods enqueue asynchronous operations; they cannot return a warning about a packet received later.

Expose diagnostics without surprising existing clients

My preference is an additive, opt-in method such as monitor_diagnostics() -> Result<Receiver<PacketDiagnostic>>. Existing applications keep their current service/event APIs; applications interested in partial handling can subscribe. Make the diagnostic types extensible and delivery bounded, nonblocking, and documented as best-effort, with aggregation or dropped-event counts so noisy traffic cannot stall discovery or exhaust memory. Numeric type/count/interface information should be sufficient by default, without dumping packets or names.

A smaller alternative is a new DaemonEvent warning variant: that enum is already #[non_exhaustive], so this is source-compatible for downstream matches. However, existing monitor consumers would begin seeing additional traffic. I would avoid routing recoverable diagnostics through DaemonEvent::Error, since applications may reasonably respond to those by restarting the daemon.

There is also a distinction between API compatibility and behavior: accepting the supported part of a formerly rejected packet is an intentional behavior change. I would document it as the recovery fix. If preserving the old rejection policy is required, an explicit opt-in recovery policy can retain strict handling as the default, though existing clients would then need to opt in to get the fix.

Tests to add before revising

The current patch also needs to address fuzz_api::parse_packet, which asserts that the parsed question count equals the header count. Retaining raw unknown questions preserves that invariant. If we instead keep only a supported subset, the invariant should explicitly account for every skipped question, rather than merely weaken the assertion.

I would test typed diagnostics alongside successful A/AAAA handling, unknown-only packets, preservation of the original question section, malformed input remaining fatal, the chosen strict/recovery policy, and slow or absent diagnostic subscribers. That would make the recovery behavior observable and deliberate while keeping the existing public API usable.

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.

1 participant