diff --git a/pkg/sqlite/load.go b/pkg/sqlite/load.go index 190144eaf..f77a5d7cc 100644 --- a/pkg/sqlite/load.go +++ b/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 { diff --git a/pkg/sqlite/substitutesfor_cycle_test.go b/pkg/sqlite/substitutesfor_cycle_test.go new file mode 100644 index 000000000..48e11f96e --- /dev/null +++ b/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 := ®istry.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") +}