socket: Enforce socket length

A Unix domain socket is limited to 107 usable bytes on Linux. However,
not all code creating socket paths was checking for this limits.

Created a new `utils.BuildSocketPath()` function (with tests) to
encapsulate the logic and updated all code creating sockets to use it.

Fixes #268.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

virtcontainers/hyperstart_agent.go

@@ -739,12 +739,17 @@ func (h *hyper) register() error {
 	}
 	defer h.disconnect()
 
+	console, err := h.sandbox.hypervisor.getSandboxConsole(h.sandbox.id)
+	if err != nil {
+		return err
+	}
+
 	registerVMOptions := &proxyClient.RegisterVMOptions{
-		Console:      h.sandbox.hypervisor.getSandboxConsole(h.sandbox.id),
+		Console:      console,
 		NumIOStreams: 0,
 	}
 
-	_, err := h.client.RegisterVM(h.sandbox.id, h.sockets[0].HostPath,
+	_, err = h.client.RegisterVM(h.sandbox.id, h.sockets[0].HostPath,
 		h.sockets[1].HostPath, registerVMOptions)
 	return err
 }

virtcontainers/hypervisor.go

@@ -501,6 +501,6 @@ type hypervisor interface {
 	addDevice(devInfo interface{}, devType deviceType) error
 	hotplugAddDevice(devInfo interface{}, devType deviceType) error
 	hotplugRemoveDevice(devInfo interface{}, devType deviceType) error
-	getSandboxConsole(sandboxID string) string
+	getSandboxConsole(sandboxID string) (string, error)
 	capabilities() capabilities
 }

virtcontainers/kata_agent.go

@@ -33,7 +33,7 @@ import (
 )
 
 var (
-	defaultKataSockPathTemplate = "%s/%s/kata.sock"
+	defaultKataSocketName = "kata.sock"
 	defaultKataChannel    = "agent.channel.0"
 	defaultKataDeviceID   = "channel0"
 	defaultKataID         = "charch0"
@@ -114,10 +114,15 @@ func (k *kataAgent) generateVMSocket(sandbox *Sandbox, c KataAgentConfig) error
 	cid, port, err := parseVSOCKAddr(c.GRPCSocket)
 	if err != nil {
 		// We need to generate a host UNIX socket path for the emulated serial port.
+		kataSock, err := utils.BuildSocketPath(runStoragePath, sandbox.id, defaultKataSocketName)
+		if err != nil {
+			return err
+		}
+
 		k.vmSocket = Socket{
 			DeviceID: defaultKataDeviceID,
 			ID:       defaultKataID,
-			HostPath: fmt.Sprintf(defaultKataSockPathTemplate, runStoragePath, sandbox.id),
+			HostPath: kataSock,
 			Name:     defaultKataChannel,
 		}
 	} else {

virtcontainers/kata_builtin_proxy.go

@@ -30,8 +30,12 @@ func (p *kataBuiltInProxy) start(sandbox *Sandbox, params proxyParams) (int, str
 	}
 
 	p.sandboxID = sandbox.id
-	console := sandbox.hypervisor.getSandboxConsole(sandbox.id)
-	err := p.watchConsole(consoleProtoUnix, console, params.logger)
+	console, err := sandbox.hypervisor.getSandboxConsole(sandbox.id)
+	if err != nil {
+		return -1, "", err
+	}
+
+	err = p.watchConsole(consoleProtoUnix, console, params.logger)
 	if err != nil {
 		return -1, "", err
 	}

virtcontainers/kata_proxy.go

@@ -41,7 +41,12 @@ func (p *kataProxy) start(sandbox *Sandbox, params proxyParams) (int, string, er
 	args := []string{config.Path, "-listen-socket", proxyURL, "-mux-socket", params.agentURL}
 	if config.Debug {
 		args = append(args, "-log", "debug")
-		args = append(args, "-agent-logs-socket", sandbox.hypervisor.getSandboxConsole(sandbox.id))
+		console, err := sandbox.hypervisor.getSandboxConsole(sandbox.id)
+		if err != nil {
+			return -1, "", err
+		}
+
+		args = append(args, "-agent-logs-socket", console)
 	}
 
 	cmd := exec.Command(args[0], args[1:]...)

virtcontainers/mock_hypervisor.go

@@ -57,6 +57,6 @@ func (m *mockHypervisor) hotplugRemoveDevice(devInfo interface{}, devType device
 	return nil
 }
 
-func (m *mockHypervisor) getSandboxConsole(sandboxID string) string {
-	return ""
+func (m *mockHypervisor) getSandboxConsole(sandboxID string) (string, error) {
+	return "", nil
 }

virtcontainers/mock_hypervisor_test.go

@@ -87,7 +87,12 @@ func TestMockHypervisorGetSandboxConsole(t *testing.T) {
 
 	expected := ""
 
-	if result := m.getSandboxConsole("testSandboxID"); result != expected {
+	result, err := m.getSandboxConsole("testSandboxID")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if result != expected {
 		t.Fatalf("Got %s\nExpecting %s", result, expected)
 	}
 }

virtcontainers/qemu.go

@@ -7,6 +7,7 @@ package virtcontainers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -60,8 +61,6 @@ type qemu struct {
 
 const qmpCapErrMsg = "Failed to negoatiate QMP capabilities"
 
-const qmpSockPathSizeLimit = 107
-
 const defaultConsole = "console.sock"
 
 // agnostic list of kernel parameters
@@ -221,20 +220,49 @@ func (q *qemu) memoryTopology(sandboxConfig SandboxConfig) (govmmQemu.Memory, er
 }
 
 func (q *qemu) qmpSocketPath(socketName string) (string, error) {
+	if socketName == "" {
+		return "", errors.New("need socket name")
+	}
+
 	parentDirPath := filepath.Join(runStoragePath, q.sandbox.id)
-	if len(parentDirPath) > qmpSockPathSizeLimit {
-		return "", fmt.Errorf("Parent directory path %q is too long "+
-			"(%d characters), could not add any path for the QMP socket",
-			parentDirPath, len(parentDirPath))
+
+	dir, err := utils.BuildSocketPath(parentDirPath)
+	if err != nil {
+		return "", err
 	}
 
-	path := fmt.Sprintf("%s/%s-%s", parentDirPath, socketName, q.state.UUID)
-
-	if len(path) > qmpSockPathSizeLimit {
-		return path[:qmpSockPathSizeLimit], nil
-	}
+	name := fmt.Sprintf("%s-%s", socketName, q.state.UUID)
 
+	path, err := utils.BuildSocketPath(dir, name)
+	if err == nil {
 		return path, nil
+	}
+
+	// The socket path is too long so truncate up to a minimum length.
+
+	// The minimum path length we're prepared to use (based on current
+	// values)
+	const minNameLen = 12
+
+	dirLen := len(dir)
+
+	// '-1' is for the addition of a path separator
+	availableNameLen := utils.MaxSocketPathLen - dirLen - 1
+
+	if availableNameLen < minNameLen {
+		return "", fmt.Errorf("QMP socket name cannot be shortened: %v", name)
+	}
+
+	new := name[:availableNameLen]
+
+	q.Logger().WithFields(logrus.Fields{
+		"original-name": name,
+		"new-name":      new,
+	}).Warnf("shortening QMP socket name")
+
+	name = new
+
+	return utils.BuildSocketPath(dir, name)
 }
 
 func (q *qemu) getQemuMachine(sandboxConfig SandboxConfig) (govmmQemu.Machine, error) {
@@ -362,7 +390,12 @@ func (q *qemu) createSandbox(sandboxConfig SandboxConfig) error {
 	devices = q.arch.appendBridges(devices, q.state.Bridges)
 
 	devices = q.arch.append9PVolumes(devices, sandboxConfig.Volumes)
-	devices = q.arch.appendConsole(devices, q.getSandboxConsole(sandboxConfig.ID))
+	console, err := q.getSandboxConsole(sandboxConfig.ID)
+	if err != nil {
+		return err
+	}
+
+	devices = q.arch.appendConsole(devices, console)
 
 	if initrdPath == "" {
 		devices, err = q.appendImage(devices)
@@ -874,6 +907,6 @@ func (q *qemu) addDevice(devInfo interface{}, devType deviceType) error {
 
 // getSandboxConsole builds the path of the console where we can read
 // logs coming from the sandbox.
-func (q *qemu) getSandboxConsole(sandboxID string) string {
-	return filepath.Join(runStoragePath, sandboxID, defaultConsole)
+func (q *qemu) getSandboxConsole(sandboxID string) (string, error) {
+	return utils.BuildSocketPath(runStoragePath, sandboxID, defaultConsole)
 }

virtcontainers/qemu_test.go

@@ -254,7 +254,12 @@ func TestQemuGetSandboxConsole(t *testing.T) {
 	sandboxID := "testSandboxID"
 	expected := filepath.Join(runStoragePath, sandboxID, defaultConsole)
 
-	if result := q.getSandboxConsole(sandboxID); result != expected {
+	result, err := q.getSandboxConsole(sandboxID)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if result != expected {
 		t.Fatalf("Got %s\nExpecting %s", result, expected)
 	}
 }

virtcontainers/utils/utils.go

@@ -7,15 +7,22 @@ package utils
 
 import (
 	"crypto/rand"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
+	"path/filepath"
 )
 
 const cpBinaryName = "cp"
 
 const fileMode0755 = os.FileMode(0755)
 
+// MaxSocketPathLen is the effective maximum Unix domain socket length.
+//
+// See unix(7).
+const MaxSocketPathLen = 107
+
 // FileCopy copys files from srcPath to dstPath
 func FileCopy(srcPath, dstPath string) error {
 	if srcPath == "" {
@@ -174,3 +181,22 @@ func MakeNameID(namedType, id string, maxLen int) string {
 
 	return nameID
 }
+
+// BuildSocketPath concatenates the provided elements into a path and returns
+// it. If the resulting path is longer than the maximum permitted socket path
+// on Linux, it will return an error.
+func BuildSocketPath(elements ...string) (string, error) {
+	result := filepath.Join(elements...)
+
+	if result == "" {
+		return "", errors.New("empty path")
+	}
+
+	l := len(result)
+
+	if l > MaxSocketPathLen {
+		return "", fmt.Errorf("path too long (got %v, max %v): %s", l, MaxSocketPathLen, result)
+	}
+
+	return result, nil
+}

virtcontainers/utils/utils_test.go

@@ -8,7 +8,9 @@ package utils
 import (
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -247,5 +249,48 @@ func TestGetSCSIAddress(t *testing.T) {
 		scsiAddr, err := GetSCSIAddress(test.index)
 		assert.Nil(t, err)
 		assert.Equal(t, scsiAddr, test.expectedSCSIAddress)
+
+	}
+}
+
+func TestBuildSocketPath(t *testing.T) {
+	assert := assert.New(t)
+
+	type testData struct {
+		elems    []string
+		valid    bool
+		expected string
+	}
+
+	longPath := strings.Repeat("/a", 106/2)
+	longestPath := longPath + "a"
+	pathTooLong := filepath.Join(longestPath, "x")
+
+	data := []testData{
+		{[]string{""}, false, ""},
+
+		{[]string{"a"}, true, "a"},
+		{[]string{"/a"}, true, "/a"},
+		{[]string{"a", "b", "c"}, true, "a/b/c"},
+		{[]string{"a", "/b", "c"}, true, "a/b/c"},
+		{[]string{"/a", "b", "c"}, true, "/a/b/c"},
+		{[]string{"/a", "/b", "/c"}, true, "/a/b/c"},
+
+		{[]string{longPath}, true, longPath},
+		{[]string{longestPath}, true, longestPath},
+		{[]string{pathTooLong}, false, ""},
+	}
+
+	for i, d := range data {
+		result, err := BuildSocketPath(d.elems...)
+
+		if d.valid {
+			assert.NoErrorf(err, "test %d, data %+v", i, d)
+		} else {
+			assert.Errorf(err, "test %d, data %+v", i, d)
+		}
+
+		assert.NotNil(result)
+		assert.Equal(d.expected, result)
 	}
 }

virtcontainers/virtcontainers_test.go

@@ -75,7 +75,7 @@ func TestMain(m *testing.M) {
 	}
 	SetLogger(logger)
 
-	testDir, err = ioutil.TempDir("", "virtcontainers-tmp-")
+	testDir, err = ioutil.TempDir("", "vc-tmp-")
 	if err != nil {
 		panic(err)
 	}