Reject cyclic substitutesFor chains instead of looping until OOM (#2038)

A bundle whose olm.substitutesFor annotation references its own CSV name
(or a set of bundles forming an indirect cycle) sent the chain walks in
addSubstitutesFor into infinite loops: the skips-accumulating walk grows
its slice on every iteration until the process dies with "fatal error:
runtime: out of memory". This is how a single self-referencing bundle
in web-terminal-operator 1.7.x metadata took down IIB catalog builds
(OCPBUGS-37284).

Guard every substitutesFor chain walk with a visited set seeded with the
walk's starting node, so any cycle - direct or indirect - fails fast with
an error naming the offending bundle instead of exhausting memory.

Signed-off-by: Harald Klein <hklein@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: haklein

pkg/sqlite/load.go

@@ -162,6 +162,18 @@ func (s *sqlLoader) addOperatorBundle(tx *sql.Tx, bundle *registry.Bundle) error
 	return s.addAPIs(tx, bundle)
 }
 
+// markSeen records name in seen, erroring if it was already there. It guards
+// the substitutesFor chain walks below against cyclic references (e.g. a
+// bundle substituting for itself), which would otherwise loop until the
+// process runs out of memory (OCPBUGS-37284).
+func markSeen(seen map[string]struct{}, name string) error {
+	if _, ok := seen[name]; ok {
+		return fmt.Errorf("cyclic substitutesFor chain detected involving %q", name)
+	}
+	seen[name] = struct{}{}
+	return nil
+}
+
 func (s *sqlLoader) addSubstitutesFor(tx *sql.Tx, bundle *registry.Bundle) error {
 	updateBundleReplaces, err := tx.Prepare("update operatorbundle set replaces = ? where replaces = ?")
 	if err != nil {
@@ -217,11 +229,15 @@ func (s *sqlLoader) addSubstitutesFor(tx *sql.Tx, bundle *registry.Bundle) error
 		if err != nil {
 			return err
 		}
+		otherSeen := map[string]struct{}{csvName: {}}
 		for len(otherSubstitutions) > 0 {
 			// consume the slice of substitutions
 			otherSubstitution := otherSubstitutions[0]
 			otherSubstitutions = otherSubstitutions[1:]
 			if otherSubstitution != csvName {
+				if err := markSeen(otherSeen, otherSubstitution); err != nil {
+					return err
+				}
 				// Another bundle is substituting for that same bundle
 				// Get other bundle's version
 				_, _, rawVersion, err := s.getBundleSkipsReplacesVersion(tx, otherSubstitution)
@@ -288,7 +304,11 @@ func (s *sqlLoader) addSubstitutesFor(tx *sql.Tx, bundle *registry.Bundle) error
 
 	// If the substituted-for of the current bundle substitutes for another bundle
 	// it should also be added to the skips of the substitutesFor bundle
+	seen := map[string]struct{}{csvName: {}}
 	for substitutesFor != "" {
+		if err := markSeen(seen, substitutesFor); err != nil {
+			return err
+		}
 		skips = append(skips, substitutesFor)
 		substitutesFor, err = s.getBundleSubstitution(tx, substitutesFor)
 		if err != nil {
@@ -305,7 +325,11 @@ func (s *sqlLoader) addSubstitutesFor(tx *sql.Tx, bundle *registry.Bundle) error
 	if err != nil || len(substitutesFors) > 1 {
 		return err
 	}
+	seen = map[string]struct{}{csvName: {}}
 	for len(substitutesFors) > 0 {
+		if err := markSeen(seen, substitutesFors[0]); err != nil {
+			return err
+		}
 		err = s.appendSkips(tx, append(skips, csvName), substitutesFors[0])
 		if err != nil {
 			return err
@@ -318,6 +342,7 @@ func (s *sqlLoader) addSubstitutesFor(tx *sql.Tx, bundle *registry.Bundle) error
 
 	// Bundles that skip a bundle that is substituted for
 	// should also skip the substituted-for bundle
+	// nolint:nestif
 	if len(skips) != 0 {
 		// ensure slice of skips doesn't contain duplicates
 		substitutesSkips := make(map[string]struct{})
@@ -328,12 +353,16 @@ func (s *sqlLoader) addSubstitutesFor(tx *sql.Tx, bundle *registry.Bundle) error
 			if err != nil || len(substitutesFors) > 1 {
 				return err
 			}
+			seen = map[string]struct{}{skip: {}}
 			for len(substitutesFors) > 0 {
 				// consume the slice of substitutions
 				substitutesFor = substitutesFors[0]
 				substitutesFors = substitutesFors[1:]
 				// shouldn't skip yourself
 				if substitutesFor != csvName {
+					if err := markSeen(seen, substitutesFor); err != nil {
+						return err
+					}
 					substitutesSkips[substitutesFor] = struct{}{}
 					substitutesFors, err = s.getBundlesThatSubstitutesFor(tx, substitutesFor)
 					if err != nil || len(substitutesFors) > 1 {
@@ -356,9 +385,13 @@ func (s *sqlLoader) addSubstitutesFor(tx *sql.Tx, bundle *registry.Bundle) error
 		if err != nil {
 			return err
 		}
+		seen = map[string]struct{}{replaces: {}}
 		for len(substitutesFors) > 0 {
 			// update the replaces to a newer substitution
 			replaces = substitutesFors[0]
+			if err := markSeen(seen, replaces); err != nil {
+				return err
+			}
 			// try to get the substitution of the substitution
 			substitutesFors, err = s.getBundlesThatSubstitutesFor(tx, replaces)
 			if err != nil || len(substitutesFors) > 1 {

pkg/sqlite/substitutesfor_cycle_test.go

@@ -0,0 +1,67 @@
+package sqlite
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+
+	"github.com/operator-framework/operator-registry/pkg/registry"
+)
+
+func newUnstructuredCSVWithSubstitutesFor(t *testing.T, name, substitutesFor string) *unstructured.Unstructured {
+	csv := &registry.ClusterServiceVersion{}
+	csv.Kind = "ClusterServiceVersion"
+	csv.SetName(name)
+	csv.SetAnnotations(map[string]string{"olm.substitutesFor": substitutesFor})
+	csv.Spec = json.RawMessage(fmt.Sprintf(`{"version": %q}`, "1.0.0"))
+
+	out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(csv)
+	require.NoError(t, err)
+	return &unstructured.Unstructured{Object: out}
+}
+
+func newSubstitutesForLoader(t *testing.T) (*sqlLoader, func()) {
+	db, cleanup := CreateTestDB(t)
+	store, err := NewSQLLiteLoader(db, WithEnableAlpha(true))
+	require.NoError(t, err)
+	require.NoError(t, store.Migrate(context.TODO()))
+	return store.(*sqlLoader), cleanup
+}
+
+// Reproducer for OCPBUGS-37284: a bundle whose olm.substitutesFor annotation
+// references its own CSV name sends addSubstitutesFor into an unbounded chain
+// walk that grows the skips slice until the process runs out of memory.
+// It must be rejected with an error instead.
+func TestSubstitutesForSelfReference(t *testing.T) {
+	loader, cleanup := newSubstitutesForLoader(t)
+	defer cleanup()
+
+	self := newBundle(t, "csv-self", "pkg", []string{"stable"},
+		newUnstructuredCSVWithSubstitutesFor(t, "csv-self", "csv-self"))
+
+	err := loader.AddOperatorBundle(self)
+	require.ErrorContains(t, err, "cyclic substitutesFor")
+}
+
+// An indirect cycle (A substitutes for B, B substitutes for A) must also be
+// rejected instead of looping forever.
+func TestSubstitutesForIndirectCycle(t *testing.T) {
+	loader, cleanup := newSubstitutesForLoader(t)
+	defer cleanup()
+
+	a := newBundle(t, "csv-a", "pkg", []string{"stable"},
+		newUnstructuredCSVWithSubstitutesFor(t, "csv-a", "csv-b"))
+	// csv-b does not exist yet, so adding csv-a is legal
+	require.NoError(t, loader.AddOperatorBundle(a))
+
+	b := newBundle(t, "csv-b", "pkg", []string{"stable"},
+		newUnstructuredCSVWithSubstitutesFor(t, "csv-b", "csv-a"))
+
+	err := loader.AddOperatorBundle(b)
+	require.ErrorContains(t, err, "cyclic substitutesFor")
+}