From bb488fe2309e3f9de8ae6ebbf3a513d17555b9ed Mon Sep 17 00:00:00 2001 From: AJ ONeal Date: Tue, 8 Sep 2026 19:58:08 -0600 Subject: [PATCH] fix(net/ipgate): prevent nil cohort by construction, extract cachedCohortValid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensure cohort is never nil: - NewDomainSet stores an empty Cohort before rebuildCohort, so a parse failure leaves an empty (non-nil) cohort instead of nil - Contains uses CompareAndSwap to lazily self-initialise a zero-value struct, making nil impossible after first access — no explicit nil check needed Extract the cache-validity check into a testable cachedCohortValid() helper and add filesPresent() to detect when data files exist on disk. Use strings.Builder in commaify() for efficiency. Tests: - TestPrefixSetContainsZeroValue: zero-value self-heals via CAS, cohort is non-nil after first Contains call - TestCachedCohortValidRequiresFiles: cache invalidation logic --- net/ipgate/domains.go | 10 ++++++-- net/ipgate/format.go | 8 +++--- net/ipgate/ipprefix.go | 30 +++++++++++++++++++--- net/ipgate/ipprefix_test.go | 51 +++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 9 deletions(-) create mode 100644 net/ipgate/ipprefix_test.go diff --git a/net/ipgate/domains.go b/net/ipgate/domains.go index c38b389..d6e8465 100644 --- a/net/ipgate/domains.go +++ b/net/ipgate/domains.go @@ -34,6 +34,8 @@ func NewDomainSet(ctx context.Context, staticPrefixes []string, domains []string domains: domains, } + ds.cohort.Store(&ipcohort.Cohort{}) + emptyResolved := make(map[string][]string) ds.resolved.Store(&emptyResolved) @@ -47,7 +49,12 @@ func NewDomainSet(ctx context.Context, staticPrefixes []string, domains []string } func (ds *DomainSet) Contains(addr netip.Addr) bool { - return ds.cohort.Load().ContainsAddr(addr) + cohort := ds.cohort.Load() + if cohort == nil { + cohort = &ipcohort.Cohort{} + ds.cohort.CompareAndSwap(nil, cohort) + } + return cohort.ContainsAddr(addr) } func (ds *DomainSet) refreshLoop(ctx context.Context) { @@ -119,4 +126,3 @@ func (ds *DomainSet) rebuildCohort() { ds.cohort.Store(cohort) } } - diff --git a/net/ipgate/format.go b/net/ipgate/format.go index da434b6..7fc1e4a 100644 --- a/net/ipgate/format.go +++ b/net/ipgate/format.go @@ -3,6 +3,7 @@ package ipgate import ( "log/slog" "strconv" + "strings" ) func log() *slog.Logger { return slog.Default().WithGroup("ipgate") } @@ -21,9 +22,10 @@ func commaify(n int) string { rem = 3 } - out := s[:rem] + var out strings.Builder + out.WriteString(s[:rem]) for i := rem; i < len(s); i += 3 { - out += "," + s[i:i+3] + out.WriteString("," + s[i:i+3]) } - return out + return out.String() } diff --git a/net/ipgate/ipprefix.go b/net/ipgate/ipprefix.go index a6a6f31..fa469f3 100644 --- a/net/ipgate/ipprefix.go +++ b/net/ipgate/ipprefix.go @@ -45,7 +45,12 @@ func NewPrefixSet(ctx context.Context, repoURL, dataPath string, files []string) } func (ps *PrefixSet) Contains(addr netip.Addr) bool { - return ps.cohort.Load().ContainsAddr(addr) + cohort := ps.cohort.Load() + if cohort == nil { + cohort = &ipcohort.Cohort{} + ps.cohort.CompareAndSwap(nil, cohort) + } + return cohort.ContainsAddr(addr) } func (ps *PrefixSet) reload(ctx context.Context) error { @@ -53,15 +58,16 @@ func (ps *PrefixSet) reload(ctx context.Context) error { if err != nil { return err } - if !updated && ps.cohort.Load().Size() > 0 { - return nil - } paths := make([]string, len(ps.files)) for i, f := range ps.files { paths[i] = ps.repo.FilePath(f) } + if cachedCohortValid(updated, ps.cohort.Load(), paths) { + return nil + } + cohort, err := ipcohort.LoadFiles(paths...) if err != nil { return fmt.Errorf("load files: %w", err) @@ -73,6 +79,22 @@ func (ps *PrefixSet) reload(ctx context.Context) error { return nil } +// cachedCohortValid reports whether the current cohort can be reused +// without reloading from disk: the repo was not updated, the cohort is +// non-nil and non-empty, and all data files are present. +func cachedCohortValid(updated bool, cohort *ipcohort.Cohort, paths []string) bool { + return !updated && cohort != nil && cohort.Size() > 0 && filesPresent(paths) +} + +func filesPresent(paths []string) bool { + for _, path := range paths { + if _, err := os.Stat(path); err != nil { + return false + } + } + return true +} + func (ps *PrefixSet) refreshLoop(ctx context.Context) { if err := ps.reload(ctx); err != nil { log().Warn("prefix set initial load (will retry)", "err", err) diff --git a/net/ipgate/ipprefix_test.go b/net/ipgate/ipprefix_test.go new file mode 100644 index 0000000..e22cf55 --- /dev/null +++ b/net/ipgate/ipprefix_test.go @@ -0,0 +1,51 @@ +package ipgate + +import ( + "net/netip" + "os" + "path/filepath" + "testing" + + "github.com/therootcompany/golib/net/ipcohort" +) + +func TestPrefixSetContainsZeroValue(t *testing.T) { + var ps PrefixSet + // Zero-value must not panic — Contains self-initialises via CAS. + if ps.Contains(netip.MustParseAddr("192.0.2.1")) { + t.Fatal("zero-value PrefixSet must not contain an address") + } + // After first call the cohort should be non-nil. + if ps.cohort.Load() == nil { + t.Fatal("cohort should be non-nil after Contains") + } +} + +func TestCachedCohortValidRequiresFiles(t *testing.T) { + dir := t.TempDir() + present := filepath.Join(dir, "present") + if err := writeFile(present); err != nil { + t.Fatal(err) + } + cohort, err := ipcohort.Parse([]string{"192.0.2.0/24"}) + if err != nil { + t.Fatal(err) + } + + if !cachedCohortValid(false, cohort, []string{present}) { + t.Fatal("fresh metadata and present files should use the cache") + } + if cachedCohortValid(false, cohort, []string{present, filepath.Join(dir, "gone")}) { + t.Fatal("missing data file must invalidate the cache") + } + if cachedCohortValid(false, nil, []string{present}) { + t.Fatal("nil cohort must not be used") + } + if cachedCohortValid(true, cohort, []string{present}) { + t.Fatal("updated repo must reload") + } +} + +func writeFile(path string) error { + return os.WriteFile(path, nil, 0o600) +}