Repository: vmware/govmomi
Pull request #2954 (2022-09-20 23:44:05)

vcsim: Fix RetrieveProperties path validation to avoid panic
## Description

This PR fixes vcsim retrieveProperties to avoid crash caused by invalid pathset which is a property of an array property.

Closes: #2953

## Type of change

Please mark options that are relevant:

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update
- [ ] Build related change

## How Has This Been Tested?

- [x] Add unit tests

## Checklist:

- [x] My code follows the `CONTRIBUTION`
  [guidelines](https://github.com/vmware/govmomi/blob/master/CONTRIBUTING.md) of
  this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged


Patch:
diff --git a/cns/types/enum.go b/cns/types/enum.go
index 09186eea..dc610a9b 100644
--- a/cns/types/enum.go
+++ b/cns/types/enum.go
@@ -56,7 +56,7 @@ const (
 	QuerySelectionNameTypeDataStoreAccessibility = QuerySelectionNameType("DATASTORE_ACCESSIBILITY_STATUS")
 	QuerySelectionNameTypeHealthStatus           = QuerySelectionNameType("HEALTH_STATUS")
 	QuerySelectionNameTypeDataStoreUrl           = QuerySelectionNameType("DATASTORE_URL")
-	QuerySelectionNameTypePolicyId               = QuerySelectionNameType("POLICY_ID")
+	QuerySelectionNameTypeStoragePolicyId        = QuerySelectionNameType("POLICY_ID")
 )
 
 func init() {
@@ -91,7 +91,7 @@ const (
 	CnsQuerySelectionName_DATASTORE_ACCESSIBILITY_STATUS = CnsQuerySelectionNameType("DATASTORE_ACCESSIBILITY_STATUS")
 	CnsQuerySelectionName_HEALTH_STATUS                  = CnsQuerySelectionNameType("HEALTH_STATUS")
 	CnsQuerySelectionName_DATASTORE_URL                  = CnsQuerySelectionNameType("DATASTORE_URL")
-	CnsQuerySelectionName_POLICY_ID                      = CnsQuerySelectionNameType("POLICY_ID")
+	CnsQuerySelectionName_STORAGE_POLICY_ID              = CnsQuerySelectionNameType("POLICY_ID")
 )
 
 func init() {
diff --git a/simulator/property_collector.go b/simulator/property_collector.go
index f6951afd..44112b5a 100644
--- a/simulator/property_collector.go
+++ b/simulator/property_collector.go
@@ -52,6 +52,7 @@ func NewPropertyCollector(ref types.ManagedObjectReference) object.Reference {
 
 var errMissingField = errors.New("missing field")
 var errEmptyField = errors.New("empty field")
+var errInvalidField = errors.New("invalid field")
 
 func getObject(ctx *Context, ref types.ManagedObjectReference) (reflect.Value, bool) {
 	var obj mo.Reference
@@ -180,6 +181,11 @@ func fieldValue(rval reflect.Value, p string) (interface{}, error) {
 			rval = rval.Elem()
 		}
 
+		if kind == reflect.Slice {
+			// field of array field cannot be specified
+			return nil, errInvalidField
+		}
+
 		x := ucFirst(name)
 		val := rval.FieldByName(x)
 		if !val.IsValid() {
@@ -316,6 +322,13 @@ func (rr *retrieveResult) collectFields(ctx *Context, rval reflect.Value, fields
 					Name: name,
 				}},
 			})
+		case errInvalidField:
+			content.MissingSet = append(content.MissingSet, types.MissingProperty{
+				Path: name,
+				Fault: types.LocalizedMethodFault{Fault: &types.InvalidProperty{
+					Name: name,
+				}},
+			})
 		}
 	}
 }


Test patch:
diff --git a/cns/client_test.go b/cns/client_test.go
index 7b37b2ea..6147d02b 100644
--- a/cns/client_test.go
+++ b/cns/client_test.go
@@ -717,7 +717,7 @@ func TestClient(t *testing.T) {
 			string(cnstypes.CnsQuerySelectionName_VOLUME_NAME),
 			string(cnstypes.CnsQuerySelectionName_VOLUME_TYPE),
 			string(cnstypes.CnsQuerySelectionName_DATASTORE_URL),
-			string(cnstypes.CnsQuerySelectionName_POLICY_ID),
+			string(cnstypes.CnsQuerySelectionName_STORAGE_POLICY_ID),
 			string(cnstypes.CnsQuerySelectionName_HEALTH_STATUS),
 			string(cnstypes.CnsQuerySelectionName_BACKING_OBJECT_DETAILS),
 			string(cnstypes.CnsQuerySelectionName_COMPLIANCE_STATUS),
diff --git a/simulator/property_collector_test.go b/simulator/property_collector_test.go
index 22e9f682..be3ae34d 100644
--- a/simulator/property_collector_test.go
+++ b/simulator/property_collector_test.go
@@ -31,6 +31,7 @@ import (
 	"github.com/vmware/govmomi/simulator/esx"
 	"github.com/vmware/govmomi/simulator/vpx"
 	"github.com/vmware/govmomi/view"
+	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/methods"
 	"github.com/vmware/govmomi/vim25/mo"
 	"github.com/vmware/govmomi/vim25/soap"
@@ -846,6 +847,53 @@ func TestPropertyCollectorInvalidSpecName(t *testing.T) {
 	}
 }
 
+func TestPropertyCollectorInvalidProperty(t *testing.T) {
+	tests := []struct {
+		name          string
+		path          string
+		expectedFault types.BaseMethodFault
+	}{
+		{
+			"specify property of array property",
+			"config.hardware.device.key",
+			new(types.InvalidProperty),
+		},
+	}
+
+	for _, test := range tests {
+		test := test // assign to local var since loop var is reused
+
+		t.Run(test.name, func(t *testing.T) {
+			m := ESX()
+
+			Test(func(ctx context.Context, c *vim25.Client) {
+				pc := property.DefaultCollector(c)
+
+				vm := Map.Any("VirtualMachine")
+				vmRef := vm.Reference()
+
+				var vmMo mo.VirtualMachine
+				err := pc.RetrieveOne(ctx, vmRef, []string{test.path}, &vmMo)
+				if err == nil {
+					t.Fatal("expected error")
+				}
+
+				// NOTE: since soap.vimFaultError is not exported, use interface literal for assertion instead
+				fault, ok := err.(interface {
+					Fault() types.BaseMethodFault
+				})
+				if !ok {
+					t.Fatalf("err does not have fault: type: %T", err)
+				}
+
+				if reflect.TypeOf(fault.Fault()) != reflect.TypeOf(test.expectedFault) {
+					t.Errorf("expected fault: %T, actual fault: %T", test.expectedFault, fault.Fault())
+				}
+			}, m)
+		})
+	}
+}
+
 func TestPropertyCollectorRecursiveSelectSet(t *testing.T) {
 	ctx := context.Background()