fix: prevent nil dereference in Path.ECMP() and BGPPath.Prepend() by joshuafuller · Pull Request #500 · bio-routing/bio-rd

joshuafuller · 2026-03-29T17:59:38Z

Hi! We're using bio-rd as an embedded BGP speaker in a project. We ran into a crash in testing that we think may be a bug in Path.ECMP() — wanted to share our findings and a proposed fix.

How we found it

We have a scenario where two nodes each originate the same anycast prefix (10.0.99.0/24) as a StaticPathType route via LocRIB.AddPath(). When the two nodes peer over eBGP, each receives the other's copy of that prefix as a BGPPathType route. At that point the locRIB has two paths for the same prefix — one static, one BGP — and the process panics:

panic: runtime error: invalid memory address or nil pointer dereference

goroutine 144 [running]:
github.com/bio-routing/bio-rd/route.(*BGPPath).ECMP(...)
    route/bgp_path.go:229
github.com/bio-routing/bio-rd/route.(*Path).ECMP(...)
    route/path.go:70
github.com/bio-routing/bio-rd/route.(*Route).updateEqualPathCount(...)
    route/route.go:318
github.com/bio-routing/bio-rd/route.(*Route).PathSelection(...)
    route/route.go:210
github.com/bio-routing/bio-rd/routingtable/locRIB.(*LocRIB).AddPath(...)

Since this happens inside the FSM goroutine, we can't recover() from it — it kills the process.

What we think is happening

Path.ECMP() (route/path.go:67-78) switches on p.Type and calls the type-specific ECMP method on both p and q, but doesn't check that q has the same type first:

func (p *Path) ECMP(q *Path) bool {
    switch p.Type {
    case BGPPathType:
        return p.BGPPath.ECMP(q.BGPPath)  // q.BGPPath is nil if q is StaticPathType

We noticed that Path.Select() in the same file already has the guard:

func (p *Path) Select(q *Path) int8 {
    if p.Type > q.Type {
        return 1
    }
    if p.Type < q.Type {
        return -1
    }
    // ... switch on p.Type ...

So it looks like ECMP() may have been missed when that guard was added to Select().

We also noticed that AdjRIBOut.redistributePath() explicitly handles redistribution from StaticPathType to BGP, which suggests mixed path types in the same RIB is an intended use case.

Second issue: BGPPath.Prepend() with nil ASPath

While investigating, we also hit a related panic in BGPPath.Prepend() (route/bgp_path.go:655):

if len(*b.ASPath) == 0 {  // panics if b.ASPath is nil

This happens when a BGPPath is constructed without initializing ASPath — Prepend() is then called during the export pipeline and dereferences the nil pointer. A nil check before the existing length check would make this defensive.

What this PR does

Path.ECMP(): adds if p.Type != q.Type { return false } before the switch (mirrors the existing guard in Select())
BGPPath.Prepend(): initializes ASPath via types.NewASPath(nil) if nil before the existing len() check
5 test cases covering both issues

All existing tests pass. gofmt, go vet, and gocyclo (under 15) are clean. Happy to adjust the approach if you'd prefer a different fix. Thanks for maintaining bio-rd!

Copilot

Pull request overview

Fixes two panic scenarios in the route package when handling mixed path types and defensively prepending ASNs on BGP paths, aligning behavior with intended mixed-type RIB usage.

Changes:

Add a type mismatch guard in Path.ECMP() to avoid nil dereferences when comparing different PathTypes.
Initialize BGPPath.ASPath when nil in BGPPath.Prepend() to prevent nil pointer dereference.
Add a new test file with coverage for mixed path type ECMP comparisons and nil ASPath prepend behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
`route/path.go`	Adds `p.Type != q.Type` guard to `ECMP()` to avoid passing nil type-specific paths.
`route/bgp_path.go`	Adds nil initialization for `ASPath` inside `Prepend()` before dereferencing.
`route/mixed_path_type_test.go`	Introduces tests for mixed-type ECMP comparisons and `Prepend()` with nil `ASPath`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-29T18:03:21Z

+	// Must not panic
+	count := r.ECMPPathCount()
+	if count > 2 {
+		t.Errorf("unexpected ECMP count: %d", count)


TestRouteUpdateEqualPathCount_MixedTypes doesn’t currently exercise Route.updateEqualPathCount(): it never calls r.PathSelection(), so r.ecmpPaths stays at its zero value and the test will pass even if ECMP() still panics. Consider calling r.PathSelection() (or otherwise triggering updateEqualPathCount) and asserting the expected ECMPPathCount (likely 1 with StaticPathType preferred).

Suggested change

// Must not panic

count := r.ECMPPathCount()

if count > 2 {

t.Errorf("unexpected ECMP count: %d", count)

// Trigger path selection, which in turn calls updateEqualPathCount.

r.PathSelection()

// Must not panic and should result in a single ECMP path (static preferred).

count := r.ECMPPathCount()

if count != 1 {

t.Errorf("unexpected ECMP count: got %d, want 1", count)

Copilot · 2026-03-29T18:03:22Z

+	if len(*p.ASPath) == 0 {
+		t.Error("Prepend should have added an AS segment")
+	}


TestBGPPathPrepend_NilASPath asserts len(*p.ASPath) != 0, but that only checks that at least one AS_PATH segment exists, not that Prepend actually added an ASN. To validate behavior, assert on p.ASPathLen and/or that the first segment contains the prepended ASN (e.g., len((*p.ASPath)[0].ASNs) > 0 and value matches).

Suggested change

if len(*p.ASPath) == 0 {

t.Error("Prepend should have added an AS segment")

}

if p.ASPathLen == 0 {

t.Error("Prepend should increase ASPathLen")

}

if len(*p.ASPath) == 0 {

t.Error("Prepend should add at least one AS_PATH segment")

return

}

firstSeg := (*p.ASPath)[0]

if len(firstSeg.ASNs) == 0 {

t.Error("Prepend should add at least one ASN to the first AS_PATH segment")

} else if firstSeg.ASNs[0] != 64512 {

t.Errorf("Prepend should add prepended ASN 64512, got %d", firstSeg.ASNs[0])

}

Copilot · 2026-03-29T18:03:22Z

+		emptyPath := make(types.ASPath, 0)
+		b.ASPath = &emptyPath


The nil-ASPath initialization can be simplified and made more consistent with existing helpers by using types.NewASPath(nil) (or equivalent) rather than taking the address of a local empty slice. This avoids an extra allocation/copy path via insertNewASSequence() when ASPath starts nil.

Suggested change

emptyPath := make(types.ASPath, 0)

b.ASPath = &emptyPath

b.ASPath = types.NewASPath(nil)

Two nil pointer panics that crash the process when routes of different path types coexist in the same RIB prefix: Bug 1: Path.ECMP() switches on p.Type and dereferences q's type-specific field without checking that q has the same Type. When a StaticPathType path (locally originated) and BGPPathType path (BGP-learned) exist for the same prefix, Route.updateEqualPathCount() calls ECMP() on adjacent paths which panics: p.BGPPath.ECMP(q.BGPPath) where q.BGPPath is nil. Fix: add `if p.Type != q.Type { return false }` guard before the switch, matching the pattern already used in Path.Select() (lines 46-52). Bug 2: BGPPath.Prepend() dereferences b.ASPath without nil check. When a BGPPath is constructed without initializing ASPath (e.g. by an embedder originating routes as BGPPathType), the export pipeline calls Prepend() to add the local ASN, which panics on `len(*b.ASPath)`. Fix: initialize ASPath to empty if nil before the existing len check. Reproducer: two BGP speakers both originate the same prefix as StaticPathType. When they peer and exchange routes, the receiver's locRIB has both a static and BGP path for the prefix. PathSelection calls updateEqualPathCount which calls ECMP on the mixed pair → crash.

joshuafuller · 2026-03-29T18:08:20Z

Updated the PR to address the review comments:

TestRouteUpdateEqualPathCount_MixedTypes: now calls r.PathSelection() before checking ECMPPathCount(), and asserts count == 1
TestBGPPathPrepend_NilASPath: added assertions for ASPathLen, segment count, and that the prepended ASN (64512) is in the first segment
BGPPath.Prepend() nil init: switched to types.NewASPath(nil) for consistency with existing helpers

BarbarossaTM · 2026-03-30T11:59:33Z

Thanks for fix! I'll have a deeper look later (and try to disable copilot 😢 )

Copilot AI review requested due to automatic review settings March 29, 2026 17:59

Copilot started reviewing on behalf of joshuafuller March 29, 2026 18:00 View session

joshuafuller force-pushed the fix/ecmp-mixed-path-types branch from 9620347 to 7b1b8f3 Compare March 29, 2026 18:01

Copilot AI reviewed Mar 29, 2026

View reviewed changes

joshuafuller force-pushed the fix/ecmp-mixed-path-types branch from 7b1b8f3 to 38b6b9e Compare March 29, 2026 18:06

joshuafuller mentioned this pull request Mar 29, 2026

Path.ECMP() panics on nil dereference when RIB contains mixed path types #501

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent nil dereference in Path.ECMP() and BGPPath.Prepend()#500

fix: prevent nil dereference in Path.ECMP() and BGPPath.Prepend()#500
joshuafuller wants to merge 1 commit into
bio-routing:masterfrom
joshuafuller:fix/ecmp-mixed-path-types

joshuafuller commented Mar 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

joshuafuller commented Mar 29, 2026

Uh oh!

BarbarossaTM commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-	if len(*p.ASPath) == 0 {
-		t.Error("Prepend should have added an AS segment")
-	}
+	if p.ASPathLen == 0 {
+		t.Error("Prepend should increase ASPathLen")
+	}
+	if len(*p.ASPath) == 0 {
+		t.Error("Prepend should add at least one AS_PATH segment")
+		return
+	}
+	firstSeg := (*p.ASPath)[0]
+	if len(firstSeg.ASNs) == 0 {
+		t.Error("Prepend should add at least one ASN to the first AS_PATH segment")
+	} else if firstSeg.ASNs[0] != 64512 {
+		t.Errorf("Prepend should add prepended ASN 64512, got %d", firstSeg.ASNs[0])
+	}

	emptyPath := make(types.ASPath, 0)
	b.ASPath = &emptyPath
	b.ASPath = types.NewASPath(nil)

Conversation

joshuafuller commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How we found it

What we think is happening

Second issue: BGPPath.Prepend() with nil ASPath

What this PR does

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

joshuafuller commented Mar 29, 2026

Uh oh!

BarbarossaTM commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joshuafuller commented Mar 29, 2026 •

edited

Loading