Repository navigation
fix: discard wrong-family IP instead of returning it from getIPOnline - #319
Merged
TimothyYe merged 1 commit intoSep 16, 2026
Merged
Conversation
getIPOnline parsed each answer straight into the function's return variable, so an address rejected by the IPv4/IPv6 family check stayed in that variable and was returned once the attempt budget ran out. The caller then cached an address of the wrong family and never reached the interface fallback, because the return value was not empty. The parsed address now lives in a local that is promoted to the return value only after the family check and the body close both succeed.
Owner
|
Thanks for your contribution! |
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.
In IPv6 mode, when the configured IP endpoint answers with an IPv4 address, godns logs that it is skipping that address and then returns it anyway, so godns hands the provider an IPv4 literal for the
AAAArecord, and the handler caches that address as if the update succeeded.pkg/lib/ip_helper.go:376parses the response body straight intoonlineIP, which is the variable the function returns. The family check just below rejects an answer of the wrong family by loggingThe online IP (%s) from %s is not IPV6, will skip it.(line 385) and callingcontinue, but nothing clearsonlineIPon the way out. The retry loop is bounded bymaxAttempts := len(helper.reqURLs) * 3at line 342, so once every attempt has been rejected the loop simply ends and line 412 returns the last rejected address. The call at line 277 insidegetCurrentIPtreats any non-empty result as success and caches it withsetCurrentIP, which means theip == ""branch at line 279, the one that logsget ip online failed. Fallback to get ip from interface if possible.and tries the interface, never runs.This is reachable without an exotic setup. In IPv6 mode without
query_interfacethe dial protocol stays plaintcp(lines 301 to 308), so a dual stack echo endpoint is frequently reached over IPv4 and answers with the IPv4 address. Anip_typethat matches neither family lands in the same place, becauseutils.IsIPv4andutils.IsIPv6both return false and every answer is skipped; theIPv4/IPv6value asked about in #203 has that shape. #203 is asking for dual stack updates, which this does not add, so I cite it only as evidence that a neither-familyip_typereaches this code in practice. The IPv4 direction is harder to trigger since that dial is forced totcp4, so I treat it as a symmetry case rather than a reported failure.The fix keeps the parsed address in a local
candidateIPand promotes it toonlineIPonly after the family check passes andresponse.Body.Close()returns without error. Nothing changes when the close succeeds. A close error was already treated as a failed attempt withcontinue, so an otherwise good answer whose close fails is now discarded like any other failed attempt, which is the consistent behavior. The trailingif onlineIP == "" { log.Error("fail to get online IP") }was unreachable at that point, sincecandidateIPis already known to be non-empty and family checked, so the assignment takes its place.TestGetIPOnlineRejectsWrongFamilyinpkg/lib/ip_helper_test.gofollows thehttptestpattern already used across the repo. Each subtest starts a server that always answers with one fixed address, then builds anIPHelperdirectly withreqURLspointing at that server andidx: -1, which is whatUpdateConfigurationsets. The table covers IPv6 mode given an IPv4 answer, IPv4 mode given an IPv6 answer, anip_typeofIPv4/IPv6that matches neither family, and both matching-family cases so the success path stays pinned.getIPOnlineis an unexported method and the fields it needs are unexported too, so the test lives in packageliband no production change was needed to make it testable.Environment: macOS 26.2, Go 1.26.4 on darwin/arm64. I did not run golangci-lint locally.
gofmtandgo vetare clean and the one comment the fix adds ends with a period forgodot.With
pkg/lib/ip_helper.goreverted to master and the new test left in place:One thing I left alone:
UpdateConfigurationatpkg/lib/ip_helper.go:43rebuildsreqURLsfrom the settings it is handed but never assigns them tohelper.configuration, which stays fixed at whatever pointer reachedGetIPHelperInstancefirst through itssync.Once. A caller that passes a different*settings.Settingsgets fresh URLs whileip_type,user_agent,query_interfaceand the Mikrotik block all keep reading the old value. No caller does that today, but nothing stops one. That one touches singleton lifetime and locking aroundconfiguration, so it does not belong in this diff. Happy to send it separately if you want it.