internal/wayland: clean up pathname socket

This is cleaner than cleaning up in internal/system as it covers the failure paths.

Signed-off-by: Ophestra <cat@gensokyo.uk>

internal/system/wayland.go

@@ -3,7 +3,6 @@ package system
 import (
 	"errors"
 	"fmt"
-	"os"
 
 	"hakurei.app/container/check"
 	"hakurei.app/hst"
@@ -63,9 +62,6 @@ func (w *waylandOp) revert(sys *I, _ *Criteria) error {
 	if w.ctx != nil {
 		hangupErr = w.ctx.Close()
 	}
-	if err := sys.remove(w.dst.String()); err != nil && !errors.Is(err, os.ErrNotExist) {
-		removeErr = err
-	}
 
 	return newOpError("wayland", errors.Join(hangupErr, removeErr), true)
 }

internal/system/wayland_test.go

@@ -36,21 +36,6 @@ func TestWaylandOp(t *testing.T) {
 			call("aclUpdate", stub.ExpectArgs{"/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland", 0xbeef, []acl.Perm{acl.Read, acl.Write, acl.Execute}}, nil, stub.UniqueError(2)),
 		}, &OpError{Op: "wayland", Err: errors.Join(stub.UniqueError(2), os.ErrInvalid)}, nil, nil},
 
-		{"remove", 0xbeef, 0xff, &waylandOp{nil,
-			m("/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland"),
-			m("/run/user/1971/wayland-0"),
-			"org.chromium.Chromium",
-			"ebf083d1b175911782d413369b64ce7c",
-		}, []stub.Call{
-			call("waylandNew", stub.ExpectArgs{m("/run/user/1971/wayland-0"), m("/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland"), "org.chromium.Chromium", "ebf083d1b175911782d413369b64ce7c"}, nil, nil),
-			call("verbosef", stub.ExpectArgs{"wayland pathname socket on %q via %q", []any{m("/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland"), m("/run/user/1971/wayland-0")}}, nil, nil),
-			call("chmod", stub.ExpectArgs{"/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland", os.FileMode(0)}, nil, nil),
-			call("aclUpdate", stub.ExpectArgs{"/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland", 0xbeef, []acl.Perm{acl.Read, acl.Write, acl.Execute}}, nil, nil),
-		}, nil, []stub.Call{
-			call("verbosef", stub.ExpectArgs{"hanging up wayland socket on %q", []any{m("/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland")}}, nil, nil),
-			call("remove", stub.ExpectArgs{"/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland"}, nil, stub.UniqueError(1)),
-		}, &OpError{Op: "wayland", Err: errors.Join(stub.UniqueError(1)), Revert: true}},
-
 		{"success", 0xbeef, 0xff, &waylandOp{nil,
 			m("/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland"),
 			m("/run/user/1971/wayland-0"),
@@ -63,7 +48,6 @@ func TestWaylandOp(t *testing.T) {
 			call("aclUpdate", stub.ExpectArgs{"/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland", 0xbeef, []acl.Perm{acl.Read, acl.Write, acl.Execute}}, nil, nil),
 		}, nil, []stub.Call{
 			call("verbosef", stub.ExpectArgs{"hanging up wayland socket on %q", []any{m("/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland")}}, nil, nil),
-			call("remove", stub.ExpectArgs{"/tmp/hakurei.1971/ebf083d1b175911782d413369b64ce7c/wayland"}, nil, nil),
 		}, nil},
 	})
 

internal/wayland/conn.go

@@ -12,18 +12,32 @@ import (
 type SecurityContext struct {
 	// Pipe with its write end passed to security-context-v1.
 	closeFds [2]int
+	// Absolute pathname the socket was bound to.
+	bindPath *check.Absolute
 }
 
 // Close releases any resources held by [SecurityContext], and prevents further
 // connections to its associated socket.
+//
+// A non-nil error has the concrete type [Error].
 func (sc *SecurityContext) Close() error {
-	if sc == nil {
+	if sc == nil || sc.bindPath == nil {
 		return os.ErrInvalid
 	}
-	return errors.Join(
+
+	e := Error{RCleanup, sc.bindPath.String(), "", errors.Join(
 		syscall.Close(sc.closeFds[1]),
 		syscall.Close(sc.closeFds[0]),
-	)
+		// there is still technically a TOCTOU here but this is internal
+		// and has access to the privileged wayland socket, so it only
+		// receives trusted input (e.g. from cmd/hakurei) anyway
+		os.Remove(sc.bindPath.String()),
+	)}
+	if e.Errno != nil {
+		return &e
+	}
+
+	return nil
 }
 
 // New creates a new security context on the Wayland display at displayPath
@@ -51,12 +65,19 @@ func New(displayPath, bindPath *check.Absolute, appID, instanceID string) (*Secu
 	} else {
 		closeFds, bindErr := securityContextBindPipe(fd, bindPath, appID, instanceID)
 		if bindErr != nil {
+			// securityContextBindPipe does not try to remove the socket during cleanup
+			closeErr := os.Remove(bindPath.String())
+			if closeErr != nil && errors.Is(closeErr, os.ErrNotExist) {
+				closeErr = nil
+			}
+
 			err = errors.Join(bindErr, // already wrapped
+				closeErr,
 				// do not leak the socket
 				syscall.Close(fd),
 			)
 		}
-		return &SecurityContext{closeFds}, err
+		return &SecurityContext{closeFds, bindPath}, err
 	}
 }
 

internal/wayland/conn_test.go

@@ -3,6 +3,7 @@ package wayland
 import (
 	"errors"
 	"os"
+	"path"
 	"reflect"
 	"syscall"
 	"testing"
@@ -11,13 +12,18 @@ import (
 )
 
 func TestSecurityContextClose(t *testing.T) {
-	t.Parallel()
+	// do not parallel: fd test not thread safe
 
 	if err := (*SecurityContext)(nil).Close(); !reflect.DeepEqual(err, os.ErrInvalid) {
 		t.Fatalf("Close: error = %v", err)
 	}
 
 	var ctx SecurityContext
+	if f, err := os.Create(path.Join(t.TempDir(), "remove")); err != nil {
+		t.Fatal(err)
+	} else {
+		ctx.bindPath = check.MustAbs(f.Name())
+	}
 	if err := syscall.Pipe2(ctx.closeFds[0:], syscall.O_CLOEXEC); err != nil {
 		t.Fatalf("Pipe: error = %v", err)
 	}
@@ -25,9 +31,15 @@ func TestSecurityContextClose(t *testing.T) {
 
 	if err := ctx.Close(); err != nil {
 		t.Fatalf("Close: error = %v", err)
+	} else if _, err = os.Stat(ctx.bindPath.String()); err == nil || !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("Did not remove %q", ctx.bindPath)
 	}
 
-	wantErr := errors.Join(syscall.EBADF, syscall.EBADF)
+	wantErr := &Error{Cause: RCleanup, Path: ctx.bindPath.String(), Errno: errors.Join(syscall.EBADF, syscall.EBADF, &os.PathError{
+		Op:   "remove",
+		Path: ctx.bindPath.String(),
+		Err:  syscall.ENOENT,
+	})}
 	if err := ctx.Close(); !reflect.DeepEqual(err, wantErr) {
 		t.Fatalf("Close: error = %#v, want %#v", err, wantErr)
 	}

internal/wayland/wayland-client-helper.h

@@ -24,6 +24,8 @@ typedef enum {
     HAKUREI_WAYLAND_HOST_SOCKET,
     /* connect for host server failed, implemented in conn.go */
     HAKUREI_WAYLAND_HOST_CONNECT,
+    /* cleanup failed, implemented in conn.go */
+    HAKUREI_WAYLAND_CLEANUP,
 } hakurei_wayland_res;
 
 hakurei_wayland_res hakurei_security_context_bind(

internal/wayland/wayland.go

@@ -14,7 +14,9 @@ package wayland
 import "C"
 import (
 	"errors"
+	"os"
 	"strings"
+	"syscall"
 )
 
 const (
@@ -83,6 +85,9 @@ const (
 	RHostSocket Res = C.HAKUREI_WAYLAND_HOST_SOCKET
 	// RHostConnect is returned if connect failed for host server. Returned by [New].
 	RHostConnect Res = C.HAKUREI_WAYLAND_HOST_CONNECT
+
+	// RCleanup is returned if cleanup fails. Returned by [SecurityContext.Close].
+	RCleanup Res = C.HAKUREI_WAYLAND_CLEANUP
 )
 
 func (e *Error) Unwrap() error   { return e.Errno }
@@ -124,6 +129,19 @@ func (e *Error) Error() string {
 	case RHostConnect:
 		return e.withPrefix("cannot connect to " + e.Host)
 
+	case RCleanup:
+		var pathError *os.PathError
+		if errors.As(e.Errno, &pathError) && pathError != nil {
+			return pathError.Error()
+		}
+
+		var errno syscall.Errno
+		if errors.As(e.Errno, &errno) && errno != 0 {
+			return "cannot close wayland close_fd pipe: " + errno.Error()
+		}
+
+		return e.withPrefix("cannot hang up wayland security_context")
+
 	default:
 		return e.withPrefix("impossible outcome") /* not reached */
 	}

internal/wayland/wayland_test.go

@@ -58,15 +58,15 @@ func TestError(t *testing.T) {
 
 		{"bind", Error{
 			Cause: RBind,
-			Path:  "/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
+			Path:  "/tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
 			Errno: stub.UniqueError(5),
-		}, "cannot bind /hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland: unique error 5 injected by the test suite"},
+		}, "cannot bind /tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland: unique error 5 injected by the test suite"},
 
 		{"listen", Error{
 			Cause: RListen,
-			Path:  "/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
+			Path:  "/tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
 			Errno: stub.UniqueError(6),
-		}, "cannot listen on /hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland: unique error 6 injected by the test suite"},
+		}, "cannot listen on /tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland: unique error 6 injected by the test suite"},
 
 		{"socket invalid", Error{
 			Cause: RSocket,
@@ -92,6 +92,27 @@ func TestError(t *testing.T) {
 			Errno: stub.UniqueError(8),
 		}, "cannot connect to /run/user/1971/wayland-1: unique error 8 injected by the test suite"},
 
+		{"cleanup", Error{
+			Cause: RCleanup,
+			Path:  "/tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
+		}, "cannot hang up wayland security_context"},
+
+		{"cleanup PathError", Error{
+			Cause: RCleanup,
+			Path:  "/tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
+			Errno: errors.Join(syscall.EINVAL, &os.PathError{
+				Op:   "remove",
+				Path: "/tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
+				Err:  stub.UniqueError(9),
+			}),
+		}, "remove /tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland: unique error 9 injected by the test suite"},
+
+		{"cleanup errno", Error{
+			Cause: RCleanup,
+			Path:  "/tmp/hakurei.0/18783d07791f2460dbbcffb76c24c9e6/wayland",
+			Errno: errors.Join(syscall.EINVAL),
+		}, "cannot close wayland close_fd pipe: invalid argument"},
+
 		{"invalid", Error{
 			Cause: 0xbad,
 		}, "impossible outcome"},