Repository: vmware/govmomi
Pull request #1933 (2020-04-15 15:12:24)

Avoid possible nil pointer dereference in guest TransferURL
Avoid using host.Config if nil, SDK doc says "This might not be available for a disconnected host."


Patch:
diff --git a/guest/file_manager.go b/guest/file_manager.go
index 62fdd827..94b58316 100644
--- a/guest/file_manager.go
+++ b/guest/file_manager.go
@@ -18,6 +18,7 @@ package guest
 
 import (
 	"context"
+	"fmt"
 	"net"
 	"net/url"
 	"sync"
@@ -150,7 +151,7 @@ func (m FileManager) TransferURL(ctx context.Context, u string) (*url.URL, error
 	c := property.DefaultCollector(m.c)
 
 	var vm mo.VirtualMachine
-	err = c.RetrieveOne(ctx, m.vm, []string{"runtime.host"}, &vm)
+	err = c.RetrieveOne(ctx, m.vm, []string{"name", "runtime.host"}, &vm)
 	if err != nil {
 		return nil, err
 	}
@@ -159,7 +160,12 @@ func (m FileManager) TransferURL(ctx context.Context, u string) (*url.URL, error
 		return turl, nil // won't matter if the VM was powered off since the call to InitiateFileTransfer will fail
 	}
 
-	props := []string{"summary.config.sslThumbprint", "config.virtualNicManagerInfo.netConfig"}
+	props := []string{
+		"name",
+		"runtime.connectionState",
+		"summary.config.sslThumbprint",
+		"config.virtualNicManagerInfo.netConfig",
+	}
 
 	var host mo.HostSystem
 	err = c.RetrieveOne(ctx, *vm.Runtime.Host, props, &host)
@@ -167,6 +173,12 @@ func (m FileManager) TransferURL(ctx context.Context, u string) (*url.URL, error
 		return nil, err
 	}
 
+	if host.Config == nil {
+		return nil, fmt.Errorf("guest TransferURL failed for vm %q (%s): host %q (%s) config==nil, connectionState==%s",
+			vm.Name, vm.Self,
+			host.Name, host.Self, host.Runtime.ConnectionState)
+	}
+
 	// prefer an ESX management IP, as the hostname used when adding to VC may not be valid for this client
 	// See also object.HostSystem.ManagementIPs which we can't use here due to import cycle
 	for _, nc := range host.Config.VirtualNicManagerInfo.NetConfig {


Test patch:
diff --git a/guest/file_manager_test.go b/guest/file_manager_test.go
new file mode 100644
index 00000000..8e1880c4
--- /dev/null
+++ b/guest/file_manager_test.go
@@ -0,0 +1,60 @@
+/*
+Copyright (c) 2020 VMware, Inc. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package guest_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/vmware/govmomi/guest"
+	"github.com/vmware/govmomi/simulator"
+	"github.com/vmware/govmomi/vim25"
+)
+
+func TestTranferURL(t *testing.T) {
+	simulator.Test(func(ctx context.Context, c *vim25.Client) {
+		vm := simulator.Map.Any("VirtualMachine").(*simulator.VirtualMachine)
+		host := simulator.Map.Get(*vm.Runtime.Host).(*simulator.HostSystem)
+
+		ops := guest.NewOperationsManager(c, vm.Reference())
+		m, err := ops.FileManager(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		turl := "https://esx:443/foo/bar"
+		u, err := m.TransferURL(ctx, turl)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if u.Hostname() != "127.0.0.1" {
+			t.Errorf("hostname=%s", u.Hostname())
+		}
+
+		host.Config = nil
+		m, err = ops.FileManager(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		u, err = m.TransferURL(ctx, turl)
+		if err == nil {
+			t.Errorf("expected error (url=%s)", u)
+		}
+		t.Log(err)
+	})
+}