Deduplicate DNS response rewriting - #739
Merged
Merged
Conversation
Extract the DNS message parser and response policy into the dependency-free tc-dns workspace crate, consume it from both WireGuard and the native NetGuard path through a checked C ABI, and link that ABI from libwgbridge.so to avoid duplicating Rust runtime code.\n\nAdd parser, ABI, panic-safety, framing, build, size, and reproducibility coverage while fixing DNS-over-TCP length updates and the inherited C parser edge cases.
kasnder
marked this pull request as ready for review
August 20, 2026 13:59
Four follow-ups on the tc-dns deduplication: * tcp.c: restore filtering of coalesced and split TCP DNS reads. The extraction skipped parse_dns_response entirely unless the read held exactly one complete frame, so a coalesced or partial recv() went unfiltered — a blocking bypass against master, which blanked the first frame in place. Every frame is parsed again; only an isolated complete frame may have its 2-byte length prefix rewritten, because trimming a coalesced or split read would discard stream bytes. * tc-dns: stop charging compression pointers against the name budget. RFC 1035's 255-octet limit applies to the uncompressed name, not its wire encoding, so counting 2 octets per pointer rejected legitimate near-maximum names reached via compression and left them unblocked. Pointer chasing stays bounded by MAX_NAME_DEPTH, MAX_NAME_LABELS and the existing bounds check. Two regression tests cover both sides: a 254-octet name reached through a pointer must be recorded and blanked, an over-long uncompressed owner name must still be rejected. * wgbridge: drop rewrite_dns_response, which became test-only once the C engine moved to the tc-dns C ABI, and retarget its tests at DnsInspector::inspect_and_rewrite. * wgbridge.gradle: assert libwgbridge.so still exports tcdns_process_response and tcdns_abi_version for each ABI. These reach the shared object only because rustc chooses to re-export them from the tc-dns rlib; a future LTO or --gc-sections change would otherwise build cleanly and fail at runtime with an UnsatisfiedLinkError. Also rewrite two let...else blocks in src/dns.rs with `?`, so `cargo clippy --all-targets -- -D warnings` passes at workspace scope. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The capi feature is not a default feature, so plain cargo test skipped the three tests covering the C ABI contract (struct layout, invalid callback tables, string termination and returned length) that the NetGuard engine now depends on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six behavior-documenting tests for response shapes the suite did not cover: CNAME chains ending in A and AAAA (with compression pointers, asserting the final answer records against the question qname), an EDNS(0) OPT pseudo-record in the additional section, an RRSIG alongside a valid A answer, a round-trip check that a blanked message is a self-consistent DNS message, and a TC-bit response cut mid-record that must fail open while still recording the answers parsed before the cut. No parser changes; all six confirmed the intended behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The isolated/coalesced/split frame decision in check_tcp_socket had no automated coverage, and it is exactly where the coalesced-read blocking bypass fixed in 9c49cc0 lived. Move the pure logic to dns_frame.c (libc-only, no JNI) so it compiles on the host, keep tcp.c's behavior identical, and add an assert-based host test covering isolated frames (shortened and not), coalesced and split reads, the zero-length and 16-bit-max frame edges, and the 9c49cc0 regression. CI compiles and runs the test with the system compiler in the existing test job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tc-dnsRust cratelibwgbridge.soas the single Rust shared libraryValidation
cargo test --workspace --locked --offline(42 passed)cargo test -p tc-dns --features capi --locked --offline(16 passed)-D warningsexternalNativeBuildGithubDebug, GitHub JVM unit tests, andassembleGithubDebuglibnetguard.sohas the expectedDT_NEEDEDentry forlibwgbridge.soCARGO_HOMEdirectories produced byte-identical native libraries and APKsPixel 8 smoke test
Tested the GitHub debug APK on Pixel 8
44230DLJH00128.RCODE=3response with zero answer/authority/additional counts; ordinary A responses remained intactwg_enabled=false: UDP HTTPS rewriting matched the WireGuard behavior; ordinary UDP A responses remained intact0x0020, matching the shortened 32-byte DNS messagewg_enabled=true; WireGuard reconnected and connectivity passedRemaining limitation
The C adapter intentionally rewrites only complete isolated DNS-over-TCP frames. Split or coalesced TCP DNS frames are passed through unchanged and do not yet have a native device harness.
Follow-up test coverage (added after review)
cargo test -p tc-dns --features capi, so the three FFI-boundary tests (C ABI struct layout, invalid callback tables, string termination and returned length) execute on every run — previously they only compiled behind the non-defaultcapifeature.tc-dns/tests/message.rs: CNAME chains ending in A/AAAA (with compression pointers; the final answer records against the question qname), EDNS(0) OPT in the additional section, RRSIG alongside a valid A, a round-trip check that a blanked message is a self-consistent DNS message, and a TC-bit response cut mid-record that fails open while still recording fully-parsed answers. All six confirmed the intended behavior; no parser changes.tcp.cintodns_frame.c(libc-only, behavior-identical) and covered by an assert-based host test — including a regression case for the coalesced-read blocking bypass fixed in 9c49cc0 — compiled and run in CI with the system compiler.