fix(tun): stop reusing FakeDNS UDP receive buffer for response construction - #42
Merged
Merged
Conversation
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
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.
What
Fixes a buffer-aliasing bug in the FakeDNS UDP proxy hot loop where the single 65535-byte receive buffer's backing array was reused to build outgoing DNS responses. Under a burst of DNS queries on different hostnames, an occasional "host not found" failure could occur — only under real traffic; isolated unit tests passed because they allocate a fresh buffer per query.
Fixes the bug identified as plan-015 in the
/improveaudit backlog.Root cause
mobile/tun/fakedns_proxy.goallocates onebuf := make([]byte, 65535)outside thego func() { for { ... } }read loop and reuses it for everylocalUdp.ReadFromUDP(buf). The outgoing DNS response was built with:Because
bufhas capacity 65535,append(buf[:offset], resp...)extendsbufin place pastoffset, writing the response bytes into the receive buffer's tail. On the next iteration,ReadFromUDP(buf)overwrites only the firstnbytes (the new datagram's length); bytes pastnare left untouched — i.e. they still hold the previous iteration's response. If the new datagram is shorter than the previous response,parseDNSQuery(buf[offset:n])and the downstreambuildDNSResponsethen read offsets containing stale data from the prior reply, corrupting the new response.Symptom
Random "host not found" failures when several DNS queries for different hostnames reach the FakeDNS UDP socket in quick succession (e.g. opening a page that fans out to many ad/analytics domains). Single-query unit tests never reproduce it because each test allocates a fresh buffer.
Fix
Two defensive changes in
handleUDPAssociate's UDP read loop, both in theif tPort == 53 { ... }branch:Copy
dnsQueryout ofbufbefore parsing.parseDNSQueryandbuildDNSResponseretain a reference to the query slice's backing array; a futureReadFromUDP(buf)on the next iteration could otherwise overwrite it. The defensive copy makes the per-query handling pure.Build the outgoing response in a freshly-allocated slice sized exactly to the final length, so it cannot alias
buf:After both changes the receive buffer
bufis no longer referenced past theWriteToUDPcall, so the nextReadFromUDPcannot corrupt the outgoing response or the next query's parse.Performance
The fix adds two small heap allocations per DNS query (~hundreds of bytes each): the
dnsQuerycopy and thefullRespslice. The pre-sizedfullRespcap matches the final length, so no reallocation-growth occurs. For typical DNS volume this is negligible; if FakeDNS were ever pushed into the 10k-QPS range (extremely unlikely under single-processgomobile bind), the defensive copy could be revisited — but leave it: it's the load-bearing safety against future aliasing bugs if anyone refactorsbuildDNSResponse.Test
Adds a regression characterization
TestHandleUDPResponseBuildDoesNotReuseReceiveBufferinmobile/tun/fakedns_proxy_test.go(builds on the plan-006 test scaffolding). The test:longhostname.example.com) whose response is longer than the next datagram.append(buf[:offset], resp1...)) to mutatebuf's tail, faithfully reproducing the aliasing.ab.cd) intobufwithout clearingbuf[n:](mimickingReadFromUDP).dnsQuerycopy + freshfullRespslice) leavebuf[headerOffset:n2]untouched when building the second response.Verification
go test ./mobile/tun/...→ok masterdnsvpn-go/mobile/tun(exit 0, cache cleared; new regression test passes — verified in the executor worktree).gofmt -lon both modified files → clean (committed blobs are LF; local Windows CRLF in the working tree is normalized bycore.autocrlfon commit and does not reach Linux CI).mobile/tun/fakedns_proxy.go(+10/−2) andmobile/tun/fakedns_proxy_test.go(+100) are touched. No changes togo.mod/go.sum,mobile/mobile.go,mobile/tun/dns_mapper.go,mobile/tun/tun_api.go, or any Kotlin/Android file.