From 29f1c6f50e3ff031c35ab53187324023a9de3192 Mon Sep 17 00:00:00 2001
From: Joe Groocock <me@frebib.net>
Date: Sun, 2 May 2021 10:05:12 +0100
Subject: [PATCH] lang: types: Fix ValueOf() panic with nil pointer values

Some forms of reflect.Value can cause ValueOf() to panic when there is a
nil pointer somewhere within the reflect.Value, whether that be a
container type like a struct, list or map, or just a raw nil pointer.

In these cases, ValueOf() attempted to dereference the pointer without
ever checking if it was nil. mgmt lang doesn't have pointers of any
kind, so these Golang values cannot be represented in mcl types in the
current form so return a helpful error to the user.

Signed-off-by: Joe Groocock <me@frebib.net>
---
 lang/types/value.go      | 23 ++++++++++++++++++++++-
 lang/types/value_test.go | 18 ++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/lang/types/value.go b/lang/types/value.go
index 6331e9b5..4cdf888b 100644
--- a/lang/types/value.go
+++ b/lang/types/value.go
@@ -18,6 +18,7 @@
 package types
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"sort"
@@ -27,6 +28,16 @@ import (
 	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
+var (
+	// ErrNilValue is returned when ValueOf() attempts to represent a nil
+	// pointer as an mcl value. This is not supported in mcl.
+	ErrNilValue = errors.New("cannot represent a nil Golang value in mcl")
+
+	// ErrInvalidValue is returned when ValueOf() is called on an invalid or
+	// zero reflect.Value.
+	ErrInvalidValue = errors.New("cannot represent invalid reflect.Value")
+)
+
 // Value represents an interface to get values out of each type. It is similar
 // to the reflection interfaces used in the golang standard library.
 type Value interface {
@@ -65,12 +76,22 @@ func ValueOfGolang(i interface{}) (Value, error) {
 // a resource field. This is done with our "elvis" operator. When using this
 // function, if you pass in something with a nil value, then expect a panic or
 // an error if you're lucky.
-// FIXME: we should make sure we error instead of ever panic-ing on a nil value
 func ValueOf(v reflect.Value) (Value, error) {
+	// Gracefully handle invalid values instead of panic(). Invalid
+	// reflect.Value values can come from nil values, or the zero value.
+	if !v.IsValid() {
+		return nil, ErrInvalidValue
+	}
+
 	value := v
 	typ := value.Type()
 	kind := typ.Kind()
 	for kind == reflect.Ptr {
+		// Prevent panic() if value is a nil pointer and return an error.
+		if value.IsNil() {
+			return nil, ErrNilValue
+		}
+
 		typ = typ.Elem() // un-nest one pointer
 		kind = typ.Kind()
 
diff --git a/lang/types/value_test.go b/lang/types/value_test.go
index 2b3487b2..1fc57482 100644
--- a/lang/types/value_test.go
+++ b/lang/types/value_test.go
@@ -20,6 +20,7 @@
 package types
 
 import (
+	"errors"
 	"fmt"
 	"math"
 	"reflect"
@@ -694,6 +695,23 @@ func TestValueOf2(t *testing.T) {
 	}
 }
 
+func TestValueOf3(t *testing.T) {
+	st := struct {
+		Ptr  *string  `lang:"ptr"`
+		Ptr2 **string `lang:"ptr2"`
+	}{nil, nil}
+
+	// cannot represent nil pointers, expect an err
+	val, err := ValueOf(reflect.ValueOf(st))
+	if err == nil {
+		t.Errorf("function ValueOf(%+v) returned a Value when error was expected: %s", st, val)
+		return
+	}
+	if !errors.Is(err, ErrNilValue) {
+		t.Errorf("function ValueOf(%+v) returned err %s but types.ErrNilValue was expected", st, err)
+	}
+}
+
 func TestValueInto0(t *testing.T) {
 	// converts a Go variable to a types.Value, or panics if any error
 	mustValue := func(v interface{}) Value {