Skip to content

Deleted#4657

Closed
patterniha wants to merge 2 commits intoXTLS:mainfrom
patterniha:dns-bugs
Closed

Deleted#4657
patterniha wants to merge 2 commits intoXTLS:mainfrom
patterniha:dns-bugs

Conversation

@patterniha
Copy link
Collaborator

@patterniha patterniha commented Apr 28, 2025

similar to #4611 but new-features removed.

so, after this PR merged, i open new PR for new-features.

fixed bugs and refactor and optimization are related and cannot be separated further.

Fixed bugs:

  1. When the cache is disabled (disableCache = true), instead of not using the cache and sending a new IP-query, it both uses the cache and sends new IP-Query!
    This is because the order of the codes in nameserver_xxx-QueryIP function is wrong and the ips, ttl, err := s.findIPsForDomain(fqdn, option) should be after select-case code. link
    also we should not use for loop for this part of code.

  2. Suppose dialing a dns-server is unsuccessful(for tcp/quic base DNS)[for example receiving rst-ack after sending syn or receiving http-error response for doh], Instead of immediately returning an error and trying the next DNS-server-fallback, it waits until the timeout ends and then tries the next fallback!

  3. When ipOption.IPv4Enable and ipOption.IPv6Enable are both true, two IP-Query(A, AAAA) are sent and it waits for both responses to be received, then it merges the responses and returns, but suppose only AAAA-response is received and in the meantime, another request comes, while the first request is still waiting for A-response, For the second request, since AAAA-record is in the cache, it uses the cache and incorrectly only v6-IPs is returned for the second request! while the second request must wait for A-response like the first request.
    also, suppose the A-record expire sooner than AAAA-record, so So until AAAA-record expires we only have IPv6!
    also, suppose we receive only AAAA-response and the A-response dropped for a request,
    so for all subsequent requests and for 600 seconds we only have ipv6! and if our network is IPv4 only, for 600 seconds we cannot access internet!!!
    this problem affect internal usage of built-in-DNS (for example domainStrategy = "UseIP" or domainStrategy = "IPOnDemand" for routing) but not affect client/browser request, because for client/browser IP-request we have two distinct request for IPv4 and IPv6(pass through dns-proxy) but for UseIP/IPOnDemand requests we have one merged request, and this bug only affect merged requests.

  4. when IP-record-until-expire-time is less than 1 seconds and the IP-list is not empty, it returns IPs with TTL = 0 but we shouldn't set record-TTL to 0. The number 0 is not defined in the standard, and it may cause DNS information to be ignored or rejected,
    so after converting to uint32, It should be rounded up, not down. link

  5. the IsOwnLink function in "app > dns > dns.go" not updated after adding tag for each DNS-server.

  6. instead of creating new GeoIPMatcherContainer in "dns.go" we should use GlobalGeoIPContainer to reduce memory usage. GeoIPMatcherContainer is implemented to reduce memory usage, so we should only have one instance of GeoIPMatcherContainer in the entire code.

  7. when no IPs match expectedIPs, we should return ErrEmptyResponse instead of errExpectedIPNonMatch,
    otherwise, DNS-proxy send no response to client/browser DNS-query.

  8. in multi_error.go > AllEqual errors compare with == instead of errors.Is !

  9. CleanUP task should start(check-executed) in addPendingRequest function in nameserver_udp.go

refactor:

Most codes in nameserver files are duplicated, Cleanup, findIPsForDomain, updateIP, ...

These functions control how to read and write in the cache.

so I add CacheController struct to implement these functions in one place, and each tcp/quic/udp/doh-NameServer struct has a CacheController.

So now the codes are standard, and these function are only written once.

optimization:

Also, some optimizations have been made which are clear in the code and do not need to be explained.

for example in

ips, ttl, err := c.server.QueryIP(ctx, domain, c.clientIP, option, disableCache)

you pass c.clientIP and disableCache as function arguments, but these are fixed-options and should not pass as function arguments each time "QueryIP" call.

commit 96d7f97
Merge: 1f157e2 58c4866
Author: patterniha <71074308+patterniha@users.noreply.github.com>
Date:   Mon Apr 28 13:55:06 2025 +0330

    Merge branch 'XTLS:main' into main

commit 1f157e2
Merge: 9880e38 a608c5a
Author: patterniha <71074308+patterniha@users.noreply.github.com>
Date:   Mon Apr 28 11:27:50 2025 +0330

    Merge branch 'XTLS:main' into main

commit 9880e38
Author: patterniha <peyhsh@gmail.com>
Date:   Wed Apr 23 09:23:23 2025 +0330

    no changes, just for recheck

commit f385712
Merge: 8eecef6 922ae98
Author: patterniha <71074308+patterniha@users.noreply.github.com>
Date:   Wed Apr 23 09:10:53 2025 +0330

    Merge branch 'XTLS:main' into main

commit 8eecef6
Author: patterniha <peyhsh@gmail.com>
Date:   Sun Apr 20 13:02:23 2025 +0330

    minor optimization

commit d52a025
Merge: 3aa1d84 907a182
Author: patterniha <71074308+patterniha@users.noreply.github.com>
Date:   Sun Apr 20 12:17:44 2025 +0330

    Merge branch 'XTLS:main' into main

commit 3aa1d84
Author: patterniha <peyhsh@gmail.com>
Date:   Sun Apr 20 12:17:12 2025 +0330

    a minor optimization

commit 2a6f1be
Merge: d7248a2 0995fa4
Author: patterniha <71074308+patterniha@users.noreply.github.com>
Date:   Fri Apr 18 12:24:46 2025 +0330

    Merge branch 'XTLS:main' into main

commit d7248a2
Author: patterniha <peyhsh@gmail.com>
Date:   Thu Apr 17 04:47:26 2025 +0330

    add cache_controller

commit 021ab4a
Author: patterniha <peyhsh@gmail.com>
Date:   Tue Apr 15 13:22:55 2025 +0330

    update test files

commit 85c6abc
Author: patterniha <peyhsh@gmail.com>
Date:   Tue Apr 15 13:06:33 2025 +0330

    upgrade cache ability

commit 04346f8
Merge: 080e010 7a2f42f
Author: patterniha <71074308+patterniha@users.noreply.github.com>
Date:   Tue Apr 15 12:58:33 2025 +0330

    Merge branch 'XTLS:main' into main

commit 080e010
Author: patterniha <peyhsh@gmail.com>
Date:   Mon Apr 14 01:35:48 2025 +0330

    compare errors with "Is" instead of "=="

commit d36bd5c
Author: patterniha <peyhsh@gmail.com>
Date:   Sun Apr 13 22:03:55 2025 +0330

    fix bugs

commit 25918a5
Author: patterniha <peyhsh@gmail.com>
Date:   Sun Apr 13 16:15:50 2025 +0330

    update test files

commit e40d108
Author: patterniha <peyhsh@gmail.com>
Date:   Sun Apr 13 16:12:33 2025 +0330

    update test files

commit 8cdd362
Author: patterniha <peyhsh@gmail.com>
Date:   Sun Apr 13 16:07:33 2025 +0330

    update test files

commit 1a8711d
Author: patterniha <peyhsh@gmail.com>
Date:   Sun Apr 13 13:40:34 2025 +0330

    DNS: Reviewed
@patterniha patterniha changed the title DNS: fix bugs, redesign, optimization DNS: fix bugs, refactor, optimization Apr 28, 2025
@patterniha patterniha closed this Apr 28, 2025
@patterniha patterniha changed the title DNS: fix bugs, refactor, optimization Deleted Apr 28, 2025
@patterniha patterniha deleted the dns-bugs branch April 28, 2025 14:11
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