Fix crash in unmarkDeepWithPathsDeprecated (#3105)

Signed-off-by: James Humphries <james@james-humphries.co.uk>
Signed-off-by: James Humphries <James@james-humphries.co.uk>
Co-authored-by: Andrei Ciobanu <andrei.ciobanu@opentofu.org>

CHANGELOG.md

@@ -23,6 +23,7 @@ BUG FIXES:
 * Allow function calls in test variable blocks ([#2947](https://github.com/opentofu/opentofu/pull/2947))
 * The `issensitive` function now returns an unknown result when its argument is unknown, since a sensitive unknown value can potentially become non-sensitive once more information is available. ([#3008](https://github.com/opentofu/opentofu/pull/3008))
 * Provider references like "null.some_alias[each.key]" in .tf.json files are now correctly parsed ([#2915](https://github.com/opentofu/opentofu/issues/2915))
+* Fixed crash when processing multiple deprecated marks on a complex object ([#3105](https://github.com/opentofu/opentofu/pull/3105))
 
 ## Previous Releases
 

internal/command/e2etest/automation_test.go

@@ -244,36 +244,3 @@ func TestPlanOnlyInAutomation(t *testing.T) {
 	}
 }
 
-func TestPlanOnDeprecated(t *testing.T) {
-	t.Parallel()
-
-	fixturePath := filepath.Join("testdata", "deprecated-values")
-	tf := e2e.NewBinary(t, tofuBin, fixturePath)
-
-	//// INIT
-	_, stderr, err := tf.Run("init", "-input=false")
-	if err != nil {
-		t.Fatalf("unexpected init error: %s\nstderr:\n%s", err, stderr)
-	}
-
-	//// PLAN
-	stdout, stderr, err := tf.Run("plan")
-	if err != nil {
-		t.Fatalf("unexpected plan error: %s\nstderr:\n%s", err, stderr)
-	}
-
-	expected := []string{
-		`Variable marked as deprecated by the module author`,
-		`Variable "input" is marked as deprecated with the following message`,
-		`This var is deprecated`,
-		`Value derived from a deprecated source`,
-		`This value is derived from module.call.output, which is deprecated with the`,
-		`following message:`,
-		`this output is deprecated`,
-	}
-	for _, want := range expected {
-		if !strings.Contains(stdout, want) {
-			t.Errorf("invalid plan output. expected to contain %q but it does not:\n%s", want, stdout)
-		}
-	}
-}

internal/command/e2etest/plan_test.go

@@ -94,3 +94,77 @@ found no differences, so no changes are needed.
 		}
 	})
 }
+
+func TestPlanOnDeprecated(t *testing.T) {
+	t.Parallel()
+
+	fixturePath := filepath.Join("testdata", "deprecated-values")
+	tf := e2e.NewBinary(t, tofuBin, fixturePath)
+
+	//// INIT
+	_, stderr, err := tf.Run("init", "-input=false")
+	if err != nil {
+		t.Fatalf("unexpected init error: %s\nstderr:\n%s", err, stderr)
+	}
+
+	//// PLAN
+	stdout, stderr, err := tf.Run("plan")
+	if err != nil {
+		t.Fatalf("unexpected plan error: %s\nstderr:\n%s", err, stderr)
+	}
+
+	expected := []string{
+		`Variable marked as deprecated by the module author`,
+		`Variable "input" is marked as deprecated with the following message`,
+		`This var is deprecated`,
+		`Value derived from a deprecated source`,
+		`This value is derived from module.call.output, which is deprecated with the`,
+		`following message:`,
+		`this output is deprecated`,
+	}
+	for _, want := range expected {
+		if !strings.Contains(stdout, want) {
+			t.Errorf("invalid plan output. expected to contain %q but it does not:\n%s", want, stdout)
+		}
+	}
+}
+
+func TestPlanOnMultipleDeprecatedMarksSliceBug(t *testing.T) {
+	t.Parallel()
+
+	// Test for [the bug](https://github.com/opentofu/opentofu/issues/3104) where modifying 
+	// pathMarks slice during iteration would cause slice bounds errors when multiple 
+	// deprecated marks exist
+	fixturePath := filepath.Join("testdata", "multiple-deprecated-marks-slice-bug")
+	tf := e2e.NewBinary(t, tofuBin, fixturePath)
+
+	t.Run("multiple deprecated marks slice bug", func(t *testing.T) {
+		_, initErr, err := tf.Run("init")
+		if err != nil {
+			t.Fatalf("expected no errors on init, got error %v: %s", err, initErr)
+		}
+
+		planStdout, planErr, err := tf.Run("plan")
+		if err != nil {
+			t.Fatalf("expected no errors on plan, got error %v: %s", err, planErr)
+		}
+
+		// Should not crash and should show deprecation warnings for all outputs
+		expectedContents := []string{
+			"Changes to Outputs:",
+			"trigger = {",
+			"Value derived from a deprecated source",
+			"Use new_out1",
+			"Use new_out2", 
+			"Use new_out3",
+		}
+
+		// Strip ANSI codes for consistent testing
+		cleanOutput := stripAnsi(planStdout)
+		for _, want := range expectedContents {
+			if !strings.Contains(cleanOutput, want) {
+				t.Errorf("plan output missing expected content %q:\n%s", want, cleanOutput)
+			}
+		}
+	})
+}

internal/command/e2etest/testdata/multiple-deprecated-marks-slice-bug/main.tf

@@ -0,0 +1,20 @@
+module "test" {
+  source = "./module"
+}
+
+# This creates a SINGLE value with MULTIPLE deprecated marks at different paths
+# Each field gets its own PathValueMark with ONLY a deprecation mark
+# This pattern triggers the slice modification bug in unmarkDeepWithPathsDeprecated
+# https://github.com/opentofu/opentofu/issues/3104
+locals {
+  all_deprecated = {
+    a = module.test.out1
+    b = module.test.out2
+    c = module.test.out3
+  }
+}
+
+# Force evaluation by using in an output
+output "trigger" {
+  value = local.all_deprecated
+}

internal/command/e2etest/testdata/multiple-deprecated-marks-slice-bug/module/main.tf

@@ -0,0 +1,14 @@
+output "out1" {
+  value = "value1"
+  deprecated = "Use new_out1"
+}
+
+output "out2" {
+  value = "value2"
+  deprecated = "Use new_out2"
+}
+
+output "out3" {
+  value = "value3"
+  deprecated = "Use new_out3"
+}

internal/lang/marks/marks.go

@@ -10,9 +10,10 @@ import (
 	"strings"
 
 	"github.com/hashicorp/hcl/v2"
+	"github.com/zclconf/go-cty/cty"
+
 	"github.com/opentofu/opentofu/internal/addrs"
 	"github.com/opentofu/opentofu/internal/tfdiags"
-	"github.com/zclconf/go-cty/cty"
 )
 
 // valueMarks allow creating strictly typed values for use as cty.Value marks.
@@ -188,13 +189,16 @@ func ExtractDeprecatedDiagnosticsWithExpr(val cty.Value, expr hcl.Expression) (c
 	return val, diags
 }
 
+// unmarkDeepWithPathsDeprecated removes all deprecation marks from a value and returns them separately.
+// It returns both the input value with all deprecation marks removed whilst preserving other marks, and a slice of PathValueMarks where marks were removed.
 func unmarkDeepWithPathsDeprecated(val cty.Value) (cty.Value, []cty.PathValueMarks) {
 	unmarked, pathMarks := val.UnmarkDeepWithPaths()
 
 	var deprecationMarks []cty.PathValueMarks
+	var filteredPathMarks []cty.PathValueMarks
 
 	// Locate deprecationMarks and filter them out
-	for i, pm := range pathMarks {
+	for _, pm := range pathMarks {
 		deprecationPM := cty.PathValueMarks{
 			Path:  pm.Path,
 			Marks: make(cty.ValueMarks),
@@ -206,16 +210,15 @@ func unmarkDeepWithPathsDeprecated(val cty.Value) (cty.Value, []cty.PathValueMar
 				continue
 			}
 
-			// Remove mark from value marks
+			// Remove deprecated mark from value marks
 			delete(pm.Marks, m)
 
-			// Add mark to deprecation marks
+			// Add mark to deprecation marks to keep track of what we're removing
 			deprecationPM.Marks[m] = struct{}{}
 		}
 
-		// Remove empty path to not break caller code expectations.
-		if len(pm.Marks) == 0 {
-			pathMarks = append(pathMarks[:i], pathMarks[i+1:]...)
+		if len(pm.Marks) > 0 {
+			filteredPathMarks = append(filteredPathMarks, pm)
 		}
 
 		if len(deprecationPM.Marks) != 0 {
@@ -223,7 +226,7 @@ func unmarkDeepWithPathsDeprecated(val cty.Value) (cty.Value, []cty.PathValueMar
 		}
 	}
 
-	return unmarked.MarkWithPaths(pathMarks), deprecationMarks
+	return unmarked.MarkWithPaths(filteredPathMarks), deprecationMarks
 }
 
 func RemoveDeepDeprecated(val cty.Value) cty.Value {

internal/lang/marks/marks_test.go

@@ -11,6 +11,8 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/hashicorp/hcl/v2"
+	"github.com/zclconf/go-cty/cty"
+
 	"github.com/opentofu/opentofu/internal/addrs"
 	"github.com/opentofu/opentofu/internal/tfdiags"
 )
@@ -120,3 +122,161 @@ func TestMarkConsolidateWarnings(t *testing.T) {
 		}
 	}
 }
+
+func TestHasDeprecated(t *testing.T) {
+	tests := []struct {
+		name  string
+		input cty.Value
+		want  bool
+	}{
+		{
+			name:  "no marks",
+			input: cty.StringVal("test"),
+			want:  false,
+		},
+		{
+			name:  "only sensitive mark",
+			input: cty.StringVal("test").Mark(Sensitive),
+			want:  false,
+		},
+		{
+			name: "has deprecation mark",
+			input: Deprecated(cty.StringVal("test"), DeprecationCause{
+				By:      addrs.InputVariable{Name: "var1"},
+				Key:     "var1",
+				Message: "deprecated",
+			}),
+			want: true,
+		},
+		{
+			name: "mixed marks with deprecation",
+			input: Deprecated(cty.StringVal("test").Mark(Sensitive), DeprecationCause{
+				By:      addrs.InputVariable{Name: "var1"},
+				Key:     "var1",
+				Message: "deprecated",
+			}),
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := HasDeprecated(tt.input)
+			if got != tt.want {
+				t.Errorf("HasDeprecated() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestUnmarkDeepWithPathsDeprecated(t *testing.T) {
+	tests := []struct {
+		name                      string
+		input                     cty.Value
+		wantDeprecationPathsCount int
+	}{
+		{
+			name:                      "no marks",
+			input:                     cty.StringVal("test"),
+			wantDeprecationPathsCount: 0,
+		},
+		{
+			name: "single deprecation mark",
+			input: Deprecated(cty.StringVal("test"), DeprecationCause{
+				By:      addrs.InputVariable{Name: "var1"},
+				Key:     "var1",
+				Message: "deprecated",
+			}),
+			wantDeprecationPathsCount: 1,
+		},
+		{
+			name: "mixed marks",
+			input: Deprecated(cty.StringVal("test").Mark(Sensitive), DeprecationCause{
+				By:      addrs.InputVariable{Name: "var1"},
+				Key:     "var1",
+				Message: "deprecated",
+			}),
+			wantDeprecationPathsCount: 1,
+		},
+		{
+			name: "multiple fields all of which have only deprecation marks",
+			input: cty.ObjectVal(map[string]cty.Value{
+				"field1": Deprecated(cty.StringVal("test1"), DeprecationCause{
+					By:      addrs.InputVariable{Name: "var1"},
+					Key:     "var1",
+					Message: "deprecated1",
+				}),
+				"field2": Deprecated(cty.StringVal("test2"), DeprecationCause{
+					By:      addrs.InputVariable{Name: "var2"},
+					Key:     "var2",
+					Message: "deprecated2",
+				}),
+				"field3": Deprecated(cty.StringVal("test3"), DeprecationCause{
+					By:      addrs.InputVariable{Name: "var3"},
+					Key:     "var3",
+					Message: "deprecated3",
+				}),
+			}),
+			wantDeprecationPathsCount: 3,
+		},
+		{
+			name: "nested object with deprecation",
+			input: cty.ObjectVal(map[string]cty.Value{
+				"outer": cty.ObjectVal(map[string]cty.Value{
+					"inner": Deprecated(cty.StringVal("nested"), DeprecationCause{
+						By:      addrs.InputVariable{Name: "var1"},
+						Key:     "var1",
+						Message: "deprecated",
+					}),
+				}),
+			}),
+			wantDeprecationPathsCount: 1,
+		},
+		{
+			name: "only non-deprecation marks",
+			input: cty.ObjectVal(map[string]cty.Value{
+				"sensitive": cty.StringVal("secret").Mark(Sensitive),
+				"ephemeral": cty.StringVal("temp").Mark(Ephemeral),
+			}),
+			wantDeprecationPathsCount: 0,
+		},
+		{
+			name: "nested with other marks too",
+			input: cty.ObjectVal(map[string]cty.Value{
+				"outer": cty.ObjectVal(map[string]cty.Value{
+					"deprecated": Deprecated(cty.StringVal("dep"), DeprecationCause{
+						By:      addrs.InputVariable{Name: "var1"},
+						Key:     "var1",
+						Message: "deprecated",
+					}),
+					"sensitive": cty.StringVal("secret").Mark(Sensitive),
+				}),
+			}),
+			wantDeprecationPathsCount: 1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotUnmarked, gotDeprecationMarks := unmarkDeepWithPathsDeprecated(tt.input)
+
+			if len(gotDeprecationMarks) != tt.wantDeprecationPathsCount {
+				t.Errorf("deprecation marks count mismatch\ngot:  %d\nwant: %d", len(gotDeprecationMarks), tt.wantDeprecationPathsCount)
+			}
+
+			// Verify that the returned value has NO deprecation marks
+			if HasDeprecated(gotUnmarked) {
+				t.Error("returned value still contains deprecation marks")
+			}
+
+			// Verify all deprecation marks returned only contain deprecation marks
+			for _, pm := range gotDeprecationMarks {
+				for m := range pm.Marks {
+					if _, ok := m.(deprecationMark); !ok {
+						t.Errorf("found non-deprecation mark in deprecation marks: %T", m)
+					}
+				}
+			}
+		})
+	}
+}