From 20e45b87aef99ea092333df61cf6ea77d610f408 Mon Sep 17 00:00:00 2001
From: Pieter Noordhuis <pieter.noordhuis@databricks.com>
Date: Mon, 5 Feb 2024 17:54:41 +0100
Subject: [PATCH] Harden `dyn.Value` equality check (#1173)

## Changes

This function could panic when either side of the comparison is a nil or
empty slice. This logic is triggered when comparing the input value to
the output value when calling `dyn.Map`.

## Tests

Unit tests.
---
 libs/dyn/value.go          | 17 +++++++++++---
 libs/dyn/visit_map_test.go | 46 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/libs/dyn/value.go b/libs/dyn/value.go
index a487e13e..e9c22bfb 100644
--- a/libs/dyn/value.go
+++ b/libs/dyn/value.go
@@ -150,11 +150,22 @@ func (v Value) eq(w Value) bool {
 		// This is safe because we don't allow maps to be mutated.
 		return &v.v == &w.v
 	case KindSequence:
-		// Compare pointers to the underlying slice and slice length.
-		// This is safe because we don't allow slices to be mutated.
 		vs := v.v.([]Value)
 		ws := w.v.([]Value)
-		return &vs[0] == &ws[0] && len(vs) == len(ws)
+		lv := len(vs)
+		lw := len(ws)
+		// If both slices are empty, they are equal.
+		if lv == 0 && lw == 0 {
+			return true
+		}
+		// If they have different lengths, they are not equal.
+		if lv != lw {
+			return false
+		}
+		// They are both non-empty and have the same length.
+		// Compare pointers to the underlying slice.
+		// This is safe because we don't allow slices to be mutated.
+		return &vs[0] == &ws[0]
 	default:
 		return v.v == w.v
 	}
diff --git a/libs/dyn/visit_map_test.go b/libs/dyn/visit_map_test.go
index a5af3411..117d03f0 100644
--- a/libs/dyn/visit_map_test.go
+++ b/libs/dyn/visit_map_test.go
@@ -74,6 +74,29 @@ func TestMapFuncOnMap(t *testing.T) {
 	assert.ErrorIs(t, err, ref)
 }
 
+func TestMapFuncOnMapWithEmptySequence(t *testing.T) {
+	variants := []dyn.Value{
+		// empty sequence
+		dyn.V([]dyn.Value{}),
+		// non-empty sequence
+		dyn.V([]dyn.Value{dyn.V(42)}),
+	}
+
+	for i := 0; i < len(variants); i++ {
+		vin := dyn.V(map[string]dyn.Value{
+			"key": variants[i],
+		})
+
+		for j := 0; j < len(variants); j++ {
+			vout, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("key")), func(v dyn.Value) (dyn.Value, error) {
+				return variants[j], nil
+			})
+			assert.NoError(t, err)
+			assert.Equal(t, variants[j], vout.Get("key"))
+		}
+	}
+}
+
 func TestMapFuncOnSequence(t *testing.T) {
 	vin := dyn.V([]dyn.Value{
 		dyn.V(42),
@@ -115,6 +138,29 @@ func TestMapFuncOnSequence(t *testing.T) {
 	assert.ErrorIs(t, err, ref)
 }
 
+func TestMapFuncOnSequenceWithEmptySequence(t *testing.T) {
+	variants := []dyn.Value{
+		// empty sequence
+		dyn.V([]dyn.Value{}),
+		// non-empty sequence
+		dyn.V([]dyn.Value{dyn.V(42)}),
+	}
+
+	for i := 0; i < len(variants); i++ {
+		vin := dyn.V([]dyn.Value{
+			variants[i],
+		})
+
+		for j := 0; j < len(variants); j++ {
+			vout, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(0)), func(v dyn.Value) (dyn.Value, error) {
+				return variants[j], nil
+			})
+			assert.NoError(t, err)
+			assert.Equal(t, variants[j], vout.Index(0))
+		}
+	}
+}
+
 func TestMapForeachOnMap(t *testing.T) {
 	vin := dyn.V(map[string]dyn.Value{
 		"foo": dyn.V(42),