diff --git a/pkg/pillar/cmd/usbmanager/ioBundleTree.go b/pkg/pillar/cmd/usbmanager/ioBundleTree.go index a87b50df283..4f0aa011e4b 100644 --- a/pkg/pillar/cmd/usbmanager/ioBundleTree.go +++ b/pkg/pillar/cmd/usbmanager/ioBundleTree.go @@ -76,8 +76,14 @@ func (iobt ioBundleTree) groupParents(assigngrp string) []string { return ret } + visited := map[*ioBundlesElem]struct{}{} ioBundleElem = ioBundleElem.parent for ioBundleElem != nil { + if _, seen := visited[ioBundleElem]; seen { + // Cyclic parent chain; stop to avoid an unbounded loop. + break + } + visited[ioBundleElem] = struct{}{} ret = append(ret, ioBundleElem.assignmentGroup) ioBundleElem = ioBundleElem.parent } @@ -107,6 +113,11 @@ func (iobt ioBundleTree) groupDependendentsImpl(groups map[string]struct{}, ioBu } for _, childioBundlesElem := range ioBundlesElem.children { + if _, seen := groups[childioBundlesElem.assignmentGroup]; seen { + // Already visited this group; skip to avoid unbounded recursion on + // a cyclic parentassigngrp graph (e.g. a self-parent tree self-loop). + continue + } groups[childioBundlesElem.assignmentGroup] = struct{}{} iobt.groupDependendentsImpl(groups, childioBundlesElem) } @@ -150,6 +161,17 @@ func (iobt *ioBundleTree) removeIOBundle(ioBundle *types.IoBundle) { } func (iobt *ioBundleTree) addIOBundle(ioBundle *types.IoBundle) { + if ioBundle.AssignmentGroup != "" && + ioBundle.AssignmentGroup == ioBundle.ParentAssignmentGroup { + // A group that is its own parent would build a self-referential tree node + // (children[grp] == self), which the dependents walk would recurse on + // forever. The circular-dependency check below cannot catch this because + // a group is never among its own descendants before it is inserted. + log.Warnf("ioBundle %s has parentassigngrp equal to its assigngrp %q; "+ + "not adding to avoid a self-referential dependency", ioBundle.Phylabel, + ioBundle.AssignmentGroup) + return + } dependents := iobt.groupDependendents(ioBundle.AssignmentGroup) for _, dependee := range dependents { if dependee == ioBundle.ParentAssignmentGroup { diff --git a/pkg/pillar/cmd/usbmanager/ioBundleTree_test.go b/pkg/pillar/cmd/usbmanager/ioBundleTree_test.go index 0c989fa09f7..20705ddefe6 100644 --- a/pkg/pillar/cmd/usbmanager/ioBundleTree_test.go +++ b/pkg/pillar/cmd/usbmanager/ioBundleTree_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "testing" + "time" "unicode/utf8" "github.com/lf-edge/eve/pkg/pillar/types" @@ -600,3 +601,60 @@ func TestAddIOBundleDeadlock(t *testing.T) { iobt.addIOBundle(&ioBundles[1]) } + +// runsWithin runs fn and fails t if it does not return within d. +func runsWithin(t *testing.T, d time.Duration, fn func()) { + t.Helper() + done := make(chan struct{}) + go func() { + fn() + close(done) + }() + select { + case <-done: + case <-time.After(d): + t.Fatalf("operation did not terminate within %v (unbounded recursion/loop?)", d) + } +} + +// TestSelfParentAssigngrpRejected verifies that an ioBundle whose parentassigngrp +// equals its own assigngrp is not added to the tree. Adding it would build a +// self-referential node (children[grp] == self), and the dependents walk would +// then recurse on it forever, growing the goroutine stack until the pillar +// memory cgroup OOMs. The circular-dependency check in addIOBundle cannot catch +// this because a group is never among its own descendants before insertion. +func TestSelfParentAssigngrpRejected(t *testing.T) { + iobt := newIOBundleTree() + iobt.addIOBundle(&types.IoBundle{ + Phylabel: "phantom-parent", + AssignmentGroup: "grpx", + ParentAssignmentGroup: "grpx", + }) + if iobt.ioBundle("phantom-parent") != nil { + t.Fatal("self-parent ioBundle should have been rejected, but it was added") + } + if iobt.elementsByAssignmentGroup["grpx"] != nil { + t.Fatal("no tree element should exist for the rejected self-parent group") + } + runsWithin(t, 2*time.Second, func() { iobt.groupDependendents("grpx") }) +} + +// TestTreeWalksGuardCycles verifies the tree walks terminate even if a cyclic +// tree slips past addIOBundle: a self-loop element is built by hand and both +// walks must complete rather than recurse/loop forever. +func TestTreeWalksGuardCycles(t *testing.T) { + iobt := newIOBundleTree() + loop := &ioBundlesElem{ + ioBundlesMap: map[string]*types.IoBundle{}, + assignmentGroup: "loop", + children: map[string]*ioBundlesElem{}, + } + loop.children["loop"] = loop // self-loop in children + loop.parent = loop // self-loop in parent chain + iobt.elementsByAssignmentGroup["loop"] = loop + + runsWithin(t, 2*time.Second, func() { + iobt.groupDependendents("loop") + iobt.groupParents("loop") + }) +}