diff --git a/internal/routewatch/prefixhandler.go b/internal/routewatch/prefixhandler.go index b5ebb0c..0da2500 100644 --- a/internal/routewatch/prefixhandler.go +++ b/internal/routewatch/prefixhandler.go @@ -169,10 +169,19 @@ func (h *PrefixHandler) HandleMessage(msg *ristypes.RISMessage) { // each peer's last update for a prefix: a message that lists a prefix as // both withdrawn and announced keeps the route (RFC 4271 section 4.3). // A withdrawal names only the prefix and peer: the message's AS path, and - // so its origin AS, belongs to the announced prefixes. - for _, prefix := range msg.Withdrawals { + // so its origin AS, belongs to the announced prefixes. An update whose + // prefix does not parse is logged and dropped. + for _, withdrawn := range msg.Withdrawals { + prefix, err := canonicalPrefix(withdrawn) + if err != nil { + h.logger.Error("Dropped a withdrawal whose prefix does not parse", + "prefix", withdrawn, "peer", msg.Peer, "error", err) + + continue + } + h.batch = append(h.batch, prefixUpdate{ - prefix: canonicalPrefix(prefix), + prefix: prefix, peer: msg.Peer, messageType: "withdrawal", timestamp: timestamp, @@ -184,9 +193,18 @@ func (h *PrefixHandler) HandleMessage(msg *ristypes.RISMessage) { } for _, announcement := range msg.Announcements { - for _, prefix := range announcement.Prefixes { + for _, announced := range announcement.Prefixes { + prefix, err := canonicalPrefix(announced) + if err != nil { + h.logger.Error( + "Dropped an announcement whose prefix does not parse", + "prefix", announced, "peer", msg.Peer, "error", err) + + continue + } + h.batch = append(h.batch, prefixUpdate{ - prefix: canonicalPrefix(prefix), + prefix: prefix, originASN: originASN, peer: msg.Peer, messageType: "announcement", @@ -560,15 +578,15 @@ func (h *PrefixHandler) writeBatch( // the IP lookup builds when it looks a prefix up. The feed sends IPv6 // announcements compressed ("2001:db8::/32") but IPv6 withdrawals uncompressed // ("2001:db8:0:0:0:0:0:0/32"); stored as received, a withdrawal would not match -// the route its announcement stored. A prefix that does not parse is returned -// unchanged, and the batch flush reports it. -func canonicalPrefix(prefix string) string { +// the route its announcement stored. It returns an error for a prefix that does +// not parse. +func canonicalPrefix(prefix string) (string, error) { p, err := netip.ParsePrefix(prefix) if err != nil { - return prefix + return "", err } - return p.Masked().String() + return p.Masked().String(), nil } // parseCIDR returns the mask length and the IP version of a prefix string diff --git a/internal/routewatch/prefixhandler_internal_test.go b/internal/routewatch/prefixhandler_internal_test.go index 81aa4c7..cb17f73 100644 --- a/internal/routewatch/prefixhandler_internal_test.go +++ b/internal/routewatch/prefixhandler_internal_test.go @@ -440,6 +440,64 @@ func TestPrefixHandlerRemovesTheRouteOnAnAnnouncementWithoutAnOrigin( } } +// TestPrefixHandlerDropsAPrefixThatDoesNotParse runs an announcement of +// "not-a-prefix" and a withdrawal of "also/bad" through the prefix handler into +// a real database. Neither is stored in the prefix tables: the prefix count in +// the stats stays 0, both as kept in memory and as counted from the tables when +// the database is opened again. +func TestPrefixHandlerDropsAPrefixThatDoesNotParse(t *testing.T) { + t.Parallel() + + cfg := &config.Config{StateDir: t.TempDir()} + + db, err := database.New(cfg, logger.New()) + if err != nil { + t.Fatalf("failed to create database: %v", err) + } + + // Built without NewPrefixHandler's flush timer, so the test flushes itself. + h := &PrefixHandler{db: db, logger: logger.New()} + + withdrawal := &ristypes.RISMessage{ + Type: ristypes.MessageTypeUpdate, + Peer: testPeerIP, + ParsedTimestamp: time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC), + Withdrawals: []string{"also/bad"}, + } + h.HandleMessage(peerAnnouncement(testPeerIP, "not-a-prefix")) + h.HandleMessage(withdrawal) + h.flushBatch() + + checkNoPrefixes := func(when string) { + t.Helper() + + stats, err := db.GetStatsContext(context.Background()) + if err != nil { + t.Fatalf("GetStatsContext %s: %v", when, err) + } + + if stats.Prefixes != 0 { + t.Errorf("prefix count %s = %d, want 0", when, stats.Prefixes) + } + } + + checkNoPrefixes("after the flush") + + err = db.Close() + if err != nil { + t.Fatalf("Close: %v", err) + } + + db, err = database.New(cfg, logger.New()) + if err != nil { + t.Fatalf("failed to open the database again: %v", err) + } + + t.Cleanup(func() { _ = db.Close() }) + + checkNoPrefixes("after opening the database again") +} + // TestStateMessagesGoToThePrefixHandlerNotThePeerHandler checks that STATE // messages go to the prefix handler, which acts on a down, and not to the peer // handler, which would record a peer whose session never comes up as active.