From aab92ce3c1c6649e1a22b00ec999a271e71ef291 Mon Sep 17 00:00:00 2001 From: Ophestra Date: Wed, 19 Nov 2025 05:41:29 +0900 Subject: [PATCH] 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 --- internal/system/wayland.go | 4 ---- internal/system/wayland_test.go | 16 ------------- internal/wayland/conn.go | 29 ++++++++++++++++++++---- internal/wayland/conn_test.go | 16 +++++++++++-- internal/wayland/wayland-client-helper.h | 2 ++ internal/wayland/wayland.go | 18 +++++++++++++++ internal/wayland/wayland_test.go | 29 ++++++++++++++++++++---- 7 files changed, 84 insertions(+), 30 deletions(-) diff --git a/internal/system/wayland.go b/internal/system/wayland.go index c9e431e..8ce13b6 100644 --- a/internal/system/wayland.go +++ b/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) } diff --git a/internal/system/wayland_test.go b/internal/system/wayland_test.go index d31c117..083cc96 100644 --- a/internal/system/wayland_test.go +++ b/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}, }) diff --git a/internal/wayland/conn.go b/internal/wayland/conn.go index fa7ced8..40a909e 100644 --- a/internal/wayland/conn.go +++ b/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 } } diff --git a/internal/wayland/conn_test.go b/internal/wayland/conn_test.go index 4be6dbf..148f077 100644 --- a/internal/wayland/conn_test.go +++ b/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) } diff --git a/internal/wayland/wayland-client-helper.h b/internal/wayland/wayland-client-helper.h index 5f89ea7..ce67f6f 100644 --- a/internal/wayland/wayland-client-helper.h +++ b/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( diff --git a/internal/wayland/wayland.go b/internal/wayland/wayland.go index d0ec6cf..544f5cc 100644 --- a/internal/wayland/wayland.go +++ b/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 */ } diff --git a/internal/wayland/wayland_test.go b/internal/wayland/wayland_test.go index c5e6c55..1db5f35 100644 --- a/internal/wayland/wayland_test.go +++ b/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"},