fix(screencast): do not cache refused callers and close cached pidfds at teardown

commit: c1850b5b512a30c3aae3f11831d146be09f37fe3
author: brice · 7d ago · 2026-10-15 02:28 UTC
parent: 9f09160
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

3 files · +99 -2

M internal/adapters/screencast/bus_test.go +76 -0


@@ -2,6 +2,8 @@ package screencast
2	2
3	3		import (
4	4		"errors"
	5	+	"os"
	6	+	"slices"
5	7		"strings"
6	8		"testing"
7	9		"time"

@@ -684,3 +686,77 @@ func TestInventorySignalsGoToAllowedPeersOnly(t *testing.T) {
684	686		t.Fatalf("refused peer: %q", name)
685	687		}
686	688		}
	689	+
	690	+	// pidfdClosed reports whether the file was closed.
	691	+	func pidfdClosed(f *os.File) bool {
	692	+	_, err := f.Stat()
	693	+	return errors.Is(err, os.ErrClosed)
	694	+	}
	695	+
	696	+	// Refused callers are not cached: more of them than the cache holds still
	697	+	// leave room for the allowed peer, and their pidfds are closed at once.
	698	+	func TestDeniedCallersDoNotExhaustTheCache(t *testing.T) {
	699	+	const denied = maxCallers + 8
	700	+	// One pidfd per check: the first peer is checked twice.
	701	+	const deniedChecks = denied + 1
	702	+	h := newSvcHarness(t, harnessOpts{})
	703	+	h.peers.exeFile = func(n int) string {
	704	+	if n < deniedChecks {
	705	+	return "/usr/bin/evil"
	706	+	}
	707	+	return "/usr/bin/grim"
	708	+	}
	709	+	c0 := h.client()
	710	+	h.waitName(c0, nameIntrospect)
	711	+	for i := range denied {
	712	+	x := h.client()
	713	+	if name, _ := callErr(x, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != errAccessDenied {
	714	+	t.Fatalf("denied peer %d: %q", i, name)
	715	+	}
	716	+	// The same peer asking again is checked again, never LimitsExceeded.
	717	+	if i == 0 {
	718	+	if name, _ := callErr(x, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != errAccessDenied {
	719	+	t.Fatalf("denied peer again: %q", name)
	720	+	}
	721	+	}
	722	+	}
	723	+	good := h.client()
	724	+	if name, _ := callErr(good, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != "" {
	725	+	t.Fatalf("allowed peer after %d denied: %q", denied, name)
	726	+	}
	727	+	h.peers.mu.Lock()
	728	+	opened := slices.Clone(h.peers.opened)
	729	+	h.peers.mu.Unlock()
	730	+	for i, f := range opened[:deniedChecks] {
	731	+	if !pidfdClosed(f) {
	732	+	t.Fatalf("pidfd of denied peer %d still open", i)
	733	+	}
	734	+	}
	735	+	if pidfdClosed(opened[len(opened)-1]) {
	736	+	t.Fatal("pidfd of the allowed peer closed while it is cached")
	737	+	}
	738	+	}
	739	+
	740	+	// The pidfds of the cached callers are closed when the bus is lost.
	741	+	func TestBusLossClosesCallerPidfds(t *testing.T) {
	742	+	h := newSvcHarness(t, harnessOpts{})
	743	+	c := h.client()
	744	+	h.waitName(c, nameIntrospect)
	745	+	if name, _ := callErr(c, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != "" {
	746	+	t.Fatal(name)
	747	+	}
	748	+	h.peers.mu.Lock()
	749	+	opened := slices.Clone(h.peers.opened)
	750	+	h.peers.mu.Unlock()
	751	+	if len(opened) != 1 \|\| pidfdClosed(opened[0]) {
	752	+	t.Fatalf("%d pidfds, want one open", len(opened))
	753	+	}
	754	+	h.killBus()
	755	+	deadline := time.Now().Add(5 * time.Second)
	756	+	for !pidfdClosed(opened[0]) {
	757	+	if time.Now().After(deadline) {
	758	+	t.Fatal("pidfd of a cached caller survives the bus loss")
	759	+	}
	760	+	time.Sleep(5 * time.Millisecond)
	761	+	}
	762	+	}


@@ -2,6 +2,8 @@ package screencast
2		2
3	import (	3		import (
4	"errors"	4		"errors"
		5	+	"os"
		6	+	"slices"
5	"strings"	7		"strings"
6	"testing"	8		"testing"
7	"time"	9		"time"

@@ -684,3 +686,77 @@ func TestInventorySignalsGoToAllowedPeersOnly(t *testing.T) {
684	t.Fatalf("refused peer: %q", name)	686		t.Fatalf("refused peer: %q", name)
685	}	687		}
686	}	688		}
		689	+
		690	+	// pidfdClosed reports whether the file was closed.
		691	+	func pidfdClosed(f *os.File) bool {
		692	+	_, err := f.Stat()
		693	+	return errors.Is(err, os.ErrClosed)
		694	+	}
		695	+
		696	+	// Refused callers are not cached: more of them than the cache holds still
		697	+	// leave room for the allowed peer, and their pidfds are closed at once.
		698	+	func TestDeniedCallersDoNotExhaustTheCache(t *testing.T) {
		699	+	const denied = maxCallers + 8
		700	+	// One pidfd per check: the first peer is checked twice.
		701	+	const deniedChecks = denied + 1
		702	+	h := newSvcHarness(t, harnessOpts{})
		703	+	h.peers.exeFile = func(n int) string {
		704	+	if n < deniedChecks {
		705	+	return "/usr/bin/evil"
		706	+	}
		707	+	return "/usr/bin/grim"
		708	+	}
		709	+	c0 := h.client()
		710	+	h.waitName(c0, nameIntrospect)
		711	+	for i := range denied {
		712	+	x := h.client()
		713	+	if name, _ := callErr(x, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != errAccessDenied {
		714	+	t.Fatalf("denied peer %d: %q", i, name)
		715	+	}
		716	+	// The same peer asking again is checked again, never LimitsExceeded.
		717	+	if i == 0 {
		718	+	if name, _ := callErr(x, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != errAccessDenied {
		719	+	t.Fatalf("denied peer again: %q", name)
		720	+	}
		721	+	}
		722	+	}
		723	+	good := h.client()
		724	+	if name, _ := callErr(good, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != "" {
		725	+	t.Fatalf("allowed peer after %d denied: %q", denied, name)
		726	+	}
		727	+	h.peers.mu.Lock()
		728	+	opened := slices.Clone(h.peers.opened)
		729	+	h.peers.mu.Unlock()
		730	+	for i, f := range opened[:deniedChecks] {
		731	+	if !pidfdClosed(f) {
		732	+	t.Fatalf("pidfd of denied peer %d still open", i)
		733	+	}
		734	+	}
		735	+	if pidfdClosed(opened[len(opened)-1]) {
		736	+	t.Fatal("pidfd of the allowed peer closed while it is cached")
		737	+	}
		738	+	}
		739	+
		740	+	// The pidfds of the cached callers are closed when the bus is lost.
		741	+	func TestBusLossClosesCallerPidfds(t *testing.T) {
		742	+	h := newSvcHarness(t, harnessOpts{})
		743	+	c := h.client()
		744	+	h.waitName(c, nameIntrospect)
		745	+	if name, _ := callErr(c, nameIntrospect, pathIntrospect, ifaceIntrospect, "GetWindows", "", nil); name != "" {
		746	+	t.Fatal(name)
		747	+	}
		748	+	h.peers.mu.Lock()
		749	+	opened := slices.Clone(h.peers.opened)
		750	+	h.peers.mu.Unlock()
		751	+	if len(opened) != 1 \|\| pidfdClosed(opened[0]) {
		752	+	t.Fatalf("%d pidfds, want one open", len(opened))
		753	+	}
		754	+	h.killBus()
		755	+	deadline := time.Now().Add(5 * time.Second)
		756	+	for !pidfdClosed(opened[0]) {
		757	+	if time.Now().After(deadline) {
		758	+	t.Fatal("pidfd of a cached caller survives the bus loss")
		759	+	}
		760	+	time.Sleep(5 * time.Millisecond)
		761	+	}
		762	+	}

M internal/adapters/screencast/caller.go +17 -1


@@ -30,7 +30,8 @@ const (
30	30		// callerConfirm: a pidfd was opened; waiting for the bus to name the same
31	31		// pid again.
32	32		callerConfirm
33		-	// callerKnown: exe is the executable of the peer.
	33	+	// callerKnown: exe is the executable of the peer, and the allowlist
	34	+	// admitted it when it was proven. Only such peers are cached.
34	35		callerKnown
35	36		)
36	37

@@ -74,6 +75,12 @@ func (s Service) callerOf(name string) caller {
74	75		func (s Service) admit(req request) {
75	76		c := s.callerOf(req.sender)
76	77		if c != nil && c.state == callerKnown {
	78	+	if !s.allowed(c.exe) {
	79	+	// The policy closed on a cached peer: refuse and forget it.
	80	+	s.deny(req)
	81	+	c.release()
	82	+	return
	83	+	}
77	84		s.dispatch(c, req)
78	85		return
79	86		}

@@ -169,6 +176,15 @@ func (s Service) onBusReply(m zerobus.Message) {
169	176		s.failCaller(c, "")
170	177		return
171	178		}
	179	+	if !s.allowed(exe) {
	180	+	// A refused peer is not cached: it would hold one of the
	181	+	// maxCallers entries until it leaves the bus, and enough of
	182	+	// them would lock the real portal out. It is checked again,
	183	+	// from a free entry, if it calls again.
	184	+	s.log.Info().Str("sender", c.name).Str("exe", exe).Msg("caller refused: executable not allowed")
	185	+	s.failCaller(c, "")
	186	+	return
	187	+	}
172	188		c.exe, c.state = exe, callerKnown
173	189		s.flush(c)
174	190		}

 	// callerConfirm: a pidfd was opened; waiting for the bus to name the same
 	// pid again.
 	callerConfirm
-	// callerKnown: exe is the executable of the peer.
+	// callerKnown: exe is the executable of the peer, and the allowlist
+	// admitted it when it was proven. Only such peers are cached.
 	callerKnown
 )
 func (s *Service) admit(req *request) {
 	c := s.callerOf(req.sender)
 	if c != nil && c.state == callerKnown {
+		if !s.allowed(c.exe) {
+			// The policy closed on a cached peer: refuse and forget it.
+			s.deny(req)
+			c.release()
+			return
+		}
 		s.dispatch(c, req)
 		return
 	}
 			s.failCaller(c, "")
 			return
 		}
+		if !s.allowed(exe) {
+			// A refused peer is not cached: it would hold one of the
+			// maxCallers entries until it leaves the bus, and enough of
+			// them would lock the real portal out. It is checked again,
+			// from a free entry, if it calls again.
+			s.log.Info().Str("sender", c.name).Str("exe", exe).Msg("caller refused: executable not allowed")
+			s.failCaller(c, "")
+			return
+		}
 		c.exe, c.state = exe, callerKnown
 		s.flush(c)
 	}

M internal/adapters/screencast/screencast.go +6 -1


@@ -347,7 +347,8 @@ func (s *Service) service() {
347	347		// resources: CastClose goes to the wayland server, a frame still in flight
348	348		// is waited for (its buffer is lent to the wayland server), then the source
349	349		// is destroyed and its buffers released. Only then is PipeWire closed.
350		-	// parent is the context the service runs under; the attempt's own context
	350	+	// The pidfds of the cached callers are closed first. parent is the context
	351	+	// the service runs under; the attempt's own context
351	352		// may have ended for another reason.
352	353		//
353	354		// While parent is alive the attempt ended by itself (the bus was lost) and a

@@ -364,6 +365,10 @@ func (s *Service) service() {
364	365		// is abandoned (never reset or reused) and a new one takes its place.
365	366		func (s *Service) shutdown(parent context.Context) {
366	367		s.stopShot()
	368	+	// The cached pidfds belong to this attempt's bus connection.
	369	+	for i := range s.callers {
	370	+	s.callers[i].release()
	371	+	}
367	372		s.forEachSession(func(sess *session) { s.closeSession(sess, true) })
368	373		var deadline time.Time // set once parent ended
369	374		for {

 // resources: CastClose goes to the wayland server, a frame still in flight
 // is waited for (its buffer is lent to the wayland server), then the source
 // is destroyed and its buffers released. Only then is PipeWire closed.
-// parent is the context the service runs under; the attempt's own context
+// The pidfds of the cached callers are closed first. parent is the context
+// the service runs under; the attempt's own context
 // may have ended for another reason.
 //
 // While parent is alive the attempt ended by itself (the bus was lost) and a
 // is abandoned (never reset or reused) and a new one takes its place.
 func (s *Service) shutdown(parent context.Context) {
 	s.stopShot()
+	// The cached pidfds belong to this attempt's bus connection.
+	for i := range s.callers {
+		s.callers[i].release()
+	}
 	s.forEachSession(func(sess *session) { s.closeSession(sess, true) })
 	var deadline time.Time // set once parent ended
 	for {