fix(wayland): close the sync waiter's descriptors when it never runs
A failure in New after the sync waiter existed left its epoll set, wake eventfd and pipe read end open; a failed keymap also skipped the syncobj node. Run's failed Pin returned before the deferred close of queued waits. New now releases the waiter through one abort path and run installs that close before pinning.
- commit
- 5be01e999aaf126513e44295f37267161ade3c50
- author
- brice · · 2026-10-14 13:31 UTC
- parent
- 8d80ed0
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +60 -28
M internal/adapters/wayland/acquire_failure_test.go +29 -0
| @@ -207,6 +207,35 @@ func TestAcquirePollTerminalFailureRetiresActiveAndAdded(t *testing.T) { | |||
| 207 | 207 | } | |
| 208 | 208 | } | |
| 209 | 209 | ||
| 210 | + | // A waiter whose pipe cannot be pinned fails its waits and still closes | |
| 211 | + | // their descriptors. | |
| 212 | + | func TestSyncWaiterPinFailureClosesWaits(t *testing.T) { | |
| 213 | + | sw, err := newSyncWaiter(func() {}) | |
| 214 | + | if err != nil { | |
| 215 | + | t.Fatal(err) | |
| 216 | + | } | |
| 217 | + | defer sw.close() | |
| 218 | + | // Pinned already: run's own Pin fails with EEXIST. | |
| 219 | + | if err := sw.waiter.Pin(int32(sw.pipeR.Fd())); err != nil { | |
| 220 | + | t.Fatal(err) | |
| 221 | + | } | |
| 222 | + | // A pipe stands in for the eventfd: once run closes the read end, a | |
| 223 | + | // write fails with EPIPE, whatever reuses the descriptor number. | |
| 224 | + | var p [2]int | |
| 225 | + | if err := unix.Pipe2(p[:], unix.O_CLOEXEC|unix.O_NONBLOCK); err != nil { | |
| 226 | + | t.Fatal(err) | |
| 227 | + | } | |
| 228 | + | defer unix.Close(p[1]) | |
| 229 | + | w := sw.add(p[0]) | |
| 230 | + | sw.run(context.Background()) | |
| 231 | + | if !sw.fired(w) || !w.failed { | |
| 232 | + | t.Fatal("wait not failed after pin failure") | |
| 233 | + | } | |
| 234 | + | if _, err := unix.Write(p[1], []byte{0}); err != unix.EPIPE { | |
| 235 | + | t.Fatalf("wait descriptor still open: write %v", err) | |
| 236 | + | } | |
| 237 | + | } | |
| 238 | + | ||
| 210 | 239 | func TestFailedRootRetiresEveryChildPredecessorGraph(t *testing.T) { | |
| 211 | 240 | h := newSyncHarness(t) | |
| 212 | 241 | sw := h.s.syncWait | |
| @@ -207,6 +207,35 @@ func TestAcquirePollTerminalFailureRetiresActiveAndAdded(t *testing.T) { | |||||
| 207 | } | 207 | } | ||
| 208 | } | 208 | } | ||
| 209 | 209 | ||||
| 210 | + | // A waiter whose pipe cannot be pinned fails its waits and still closes | |||
| 211 | + | // their descriptors. | |||
| 212 | + | func TestSyncWaiterPinFailureClosesWaits(t *testing.T) { | |||
| 213 | + | sw, err := newSyncWaiter(func() {}) | |||
| 214 | + | if err != nil { | |||
| 215 | + | t.Fatal(err) | |||
| 216 | + | } | |||
| 217 | + | defer sw.close() | |||
| 218 | + | // Pinned already: run's own Pin fails with EEXIST. | |||
| 219 | + | if err := sw.waiter.Pin(int32(sw.pipeR.Fd())); err != nil { | |||
| 220 | + | t.Fatal(err) | |||
| 221 | + | } | |||
| 222 | + | // A pipe stands in for the eventfd: once run closes the read end, a | |||
| 223 | + | // write fails with EPIPE, whatever reuses the descriptor number. | |||
| 224 | + | var p [2]int | |||
| 225 | + | if err := unix.Pipe2(p[:], unix.O_CLOEXEC|unix.O_NONBLOCK); err != nil { | |||
| 226 | + | t.Fatal(err) | |||
| 227 | + | } | |||
| 228 | + | defer unix.Close(p[1]) | |||
| 229 | + | w := sw.add(p[0]) | |||
| 230 | + | sw.run(context.Background()) | |||
| 231 | + | if !sw.fired(w) || !w.failed { | |||
| 232 | + | t.Fatal("wait not failed after pin failure") | |||
| 233 | + | } | |||
| 234 | + | if _, err := unix.Write(p[1], []byte{0}); err != unix.EPIPE { | |||
| 235 | + | t.Fatalf("wait descriptor still open: write %v", err) | |||
| 236 | + | } | |||
| 237 | + | } | |||
| 238 | + | ||||
| 210 | func TestFailedRootRetiresEveryChildPredecessorGraph(t *testing.T) { | 239 | func TestFailedRootRetiresEveryChildPredecessorGraph(t *testing.T) { | ||
| 211 | h := newSyncHarness(t) | 240 | h := newSyncHarness(t) | ||
| 212 | sw := h.s.syncWait | 241 | sw := h.s.syncWait | ||
M internal/adapters/wayland/alloc_hotpath_test.go +1 -2
| @@ -64,8 +64,7 @@ func TestCommitSyncStorageIsPerUpdate(t *testing.T) { | |||
| 64 | 64 | if err != nil { | |
| 65 | 65 | t.Fatal(err) | |
| 66 | 66 | } | |
| 67 | - | defer sw.pipeR.Close() | |
| 68 | - | defer sw.waiter.Close() | |
| 67 | + | defer sw.discard() | |
| 69 | 68 | defer sw.close() | |
| 70 | 69 | srv.syncWait = sw | |
| 71 | 70 | tl := &timeline{dev: dev, handle: 7, alive: true} | |
| @@ -64,8 +64,7 @@ func TestCommitSyncStorageIsPerUpdate(t *testing.T) { | |||||
| 64 | if err != nil { | 64 | if err != nil { | ||
| 65 | t.Fatal(err) | 65 | t.Fatal(err) | ||
| 66 | } | 66 | } | ||
| 67 | - | defer sw.pipeR.Close() | 67 | + | defer sw.discard() |
| 68 | - | defer sw.waiter.Close() | |||
| 69 | defer sw.close() | 68 | defer sw.close() | ||
| 70 | srv.syncWait = sw | 69 | srv.syncWait = sw | ||
| 71 | tl := &timeline{dev: dev, handle: 7, alive: true} | 70 | tl := &timeline{dev: dev, handle: 7, alive: true} | ||
M internal/adapters/wayland/server.go +15 -16
| @@ -420,15 +420,6 @@ func New(opts Options, ch Channels, log zerowrap.Logger) (*Server, error) { | |||
| 420 | 420 | s.colorFeedbacks = map[*colorFeedback]struct{}{} | |
| 421 | 421 | s.tokens = map[string]activationToken{} | |
| 422 | 422 | s.foreignExports = map[string]*foreignExport{} | |
| 423 | - | if opts.Keymap != "" { | |
| 424 | - | fd, size, e := keymapFile(opts.Keymap) | |
| 425 | - | if e != nil { | |
| 426 | - | cleanup() | |
| 427 | - | d.Close() | |
| 428 | - | return nil, e | |
| 429 | - | } | |
| 430 | - | s.seat.keymapFD, s.seat.keymapSize, s.seat.keymapText = fd, size, opts.Keymap | |
| 431 | - | } | |
| 432 | 423 | s.syncWait.log = log | |
| 433 | 424 | s.cleanup = func() { | |
| 434 | 425 | if s.paceReady { | |
| @@ -449,20 +440,28 @@ func New(opts Options, ch Channels, log zerowrap.Logger) (*Server, error) { | |||
| 449 | 440 | } | |
| 450 | 441 | cleanup() | |
| 451 | 442 | } | |
| 452 | - | if s.securityStop, err = unix.Eventfd(0, unix.EFD_CLOEXEC|unix.EFD_NONBLOCK); err != nil { | |
| 443 | + | // Run never starts after a failure here: release what it would close. | |
| 444 | + | abort := func(err error) (*Server, error) { | |
| 453 | 445 | s.cleanup() | |
| 446 | + | s.syncWait.discard() | |
| 454 | 447 | d.Close() | |
| 455 | 448 | return nil, err | |
| 456 | 449 | } | |
| 450 | + | if opts.Keymap != "" { | |
| 451 | + | fd, size, e := keymapFile(opts.Keymap) | |
| 452 | + | if e != nil { | |
| 453 | + | return abort(e) | |
| 454 | + | } | |
| 455 | + | s.seat.keymapFD, s.seat.keymapSize, s.seat.keymapText = fd, size, opts.Keymap | |
| 456 | + | } | |
| 457 | + | if s.securityStop, err = unix.Eventfd(0, unix.EFD_CLOEXEC|unix.EFD_NONBLOCK); err != nil { | |
| 458 | + | return abort(err) | |
| 459 | + | } | |
| 457 | 460 | if err = s.openPaceTimer(); err != nil { | |
| 458 | - | s.cleanup() | |
| 459 | - | d.Close() | |
| 460 | - | return nil, err | |
| 461 | + | return abort(err) | |
| 461 | 462 | } | |
| 462 | 463 | if err = registerGlobals(d, opts, s); err != nil { | |
| 463 | - | s.cleanup() | |
| 464 | - | d.Close() | |
| 465 | - | return nil, err | |
| 464 | + | return abort(err) | |
| 466 | 465 | } | |
| 467 | 466 | for _, p := range opts.Outputs { | |
| 468 | 467 | s.addOutput(p) | |
| @@ -420,15 +420,6 @@ func New(opts Options, ch Channels, log zerowrap.Logger) (*Server, error) { | |||||
| 420 | s.colorFeedbacks = map[*colorFeedback]struct{}{} | 420 | s.colorFeedbacks = map[*colorFeedback]struct{}{} | ||
| 421 | s.tokens = map[string]activationToken{} | 421 | s.tokens = map[string]activationToken{} | ||
| 422 | s.foreignExports = map[string]*foreignExport{} | 422 | s.foreignExports = map[string]*foreignExport{} | ||
| 423 | - | if opts.Keymap != "" { | |||
| 424 | - | fd, size, e := keymapFile(opts.Keymap) | |||
| 425 | - | if e != nil { | |||
| 426 | - | cleanup() | |||
| 427 | - | d.Close() | |||
| 428 | - | return nil, e | |||
| 429 | - | } | |||
| 430 | - | s.seat.keymapFD, s.seat.keymapSize, s.seat.keymapText = fd, size, opts.Keymap | |||
| 431 | - | } | |||
| 432 | s.syncWait.log = log | 423 | s.syncWait.log = log | ||
| 433 | s.cleanup = func() { | 424 | s.cleanup = func() { | ||
| 434 | if s.paceReady { | 425 | if s.paceReady { | ||
| @@ -449,20 +440,28 @@ func New(opts Options, ch Channels, log zerowrap.Logger) (*Server, error) { | |||||
| 449 | } | 440 | } | ||
| 450 | cleanup() | 441 | cleanup() | ||
| 451 | } | 442 | } | ||
| 452 | - | if s.securityStop, err = unix.Eventfd(0, unix.EFD_CLOEXEC|unix.EFD_NONBLOCK); err != nil { | 443 | + | // Run never starts after a failure here: release what it would close. |
| 444 | + | abort := func(err error) (*Server, error) { | |||
| 453 | s.cleanup() | 445 | s.cleanup() | ||
| 446 | + | s.syncWait.discard() | |||
| 454 | d.Close() | 447 | d.Close() | ||
| 455 | return nil, err | 448 | return nil, err | ||
| 456 | } | 449 | } | ||
| 450 | + | if opts.Keymap != "" { | |||
| 451 | + | fd, size, e := keymapFile(opts.Keymap) | |||
| 452 | + | if e != nil { | |||
| 453 | + | return abort(e) | |||
| 454 | + | } | |||
| 455 | + | s.seat.keymapFD, s.seat.keymapSize, s.seat.keymapText = fd, size, opts.Keymap | |||
| 456 | + | } | |||
| 457 | + | if s.securityStop, err = unix.Eventfd(0, unix.EFD_CLOEXEC|unix.EFD_NONBLOCK); err != nil { | |||
| 458 | + | return abort(err) | |||
| 459 | + | } | |||
| 457 | if err = s.openPaceTimer(); err != nil { | 460 | if err = s.openPaceTimer(); err != nil { | ||
| 458 | - | s.cleanup() | 461 | + | return abort(err) |
| 459 | - | d.Close() | |||
| 460 | - | return nil, err | |||
| 461 | } | 462 | } | ||
| 462 | if err = registerGlobals(d, opts, s); err != nil { | 463 | if err = registerGlobals(d, opts, s); err != nil { | ||
| 463 | - | s.cleanup() | 464 | + | return abort(err) |
| 464 | - | d.Close() | |||
| 465 | - | return nil, err | |||
| 466 | } | 465 | } | ||
| 467 | for _, p := range opts.Outputs { | 466 | for _, p := range opts.Outputs { | ||
| 468 | s.addOutput(p) | 467 | s.addOutput(p) | ||
M internal/adapters/wayland/syncobj.go +13 -6
| @@ -388,12 +388,6 @@ func (sw *syncWaiter) run(ctx context.Context) { | |||
| 388 | 388 | // The pipe lives as long as the poller; the eventfds come and go. The | |
| 389 | 389 | // poll sleeps until a fence, a new or cancelled wait (the pipe), or the | |
| 390 | 390 | // end of ctx. | |
| 391 | - | sw.waiter.StopOn(ctx) | |
| 392 | - | if err := sw.waiter.Pin(int32(pipeFD)); err != nil { | |
| 393 | - | sw.log.Warn().Err(err).Msg("sync waiter") | |
| 394 | - | sw.fail(nil) | |
| 395 | - | return | |
| 396 | - | } | |
| 397 | 391 | var waits []*syncWait | |
| 398 | 392 | defer func() { | |
| 399 | 393 | sw.mu.Lock() | |
| @@ -404,6 +398,12 @@ func (sw *syncWaiter) run(ctx context.Context) { | |||
| 404 | 398 | unix.Close(w.efd) | |
| 405 | 399 | } | |
| 406 | 400 | }() | |
| 401 | + | sw.waiter.StopOn(ctx) | |
| 402 | + | if err := sw.waiter.Pin(int32(pipeFD)); err != nil { | |
| 403 | + | sw.log.Warn().Err(err).Msg("sync waiter") | |
| 404 | + | sw.fail(nil) | |
| 405 | + | return | |
| 406 | + | } | |
| 407 | 407 | for ctx.Err() == nil { | |
| 408 | 408 | live := waits[:0] | |
| 409 | 409 | for _, w := range waits { | |
| @@ -468,6 +468,13 @@ func (sw *syncWaiter) run(ctx context.Context) { | |||
| 468 | 468 | // close stops the waiter's input side. | |
| 469 | 469 | func (sw *syncWaiter) close() { sw.pipeW.Close() } | |
| 470 | 470 | ||
| 471 | + | // discard releases what run would have closed, for a waiter whose run | |
| 472 | + | // never starts. | |
| 473 | + | func (sw *syncWaiter) discard() { | |
| 474 | + | sw.pipeR.Close() | |
| 475 | + | sw.waiter.Close() | |
| 476 | + | } | |
| 477 | + | ||
| 471 | 478 | // syncHold is a buffer's explicit-sync state while it is on screen: the | |
| 472 | 479 | // acquire fence handed to readers and the release point to signal. | |
| 473 | 480 | type syncHold struct { | |
| @@ -388,12 +388,6 @@ func (sw *syncWaiter) run(ctx context.Context) { | |||||
| 388 | // The pipe lives as long as the poller; the eventfds come and go. The | 388 | // The pipe lives as long as the poller; the eventfds come and go. The | ||
| 389 | // poll sleeps until a fence, a new or cancelled wait (the pipe), or the | 389 | // poll sleeps until a fence, a new or cancelled wait (the pipe), or the | ||
| 390 | // end of ctx. | 390 | // end of ctx. | ||
| 391 | - | sw.waiter.StopOn(ctx) | |||
| 392 | - | if err := sw.waiter.Pin(int32(pipeFD)); err != nil { | |||
| 393 | - | sw.log.Warn().Err(err).Msg("sync waiter") | |||
| 394 | - | sw.fail(nil) | |||
| 395 | - | return | |||
| 396 | - | } | |||
| 397 | var waits []*syncWait | 391 | var waits []*syncWait | ||
| 398 | defer func() { | 392 | defer func() { | ||
| 399 | sw.mu.Lock() | 393 | sw.mu.Lock() | ||
| @@ -404,6 +398,12 @@ func (sw *syncWaiter) run(ctx context.Context) { | |||||
| 404 | unix.Close(w.efd) | 398 | unix.Close(w.efd) | ||
| 405 | } | 399 | } | ||
| 406 | }() | 400 | }() | ||
| 401 | + | sw.waiter.StopOn(ctx) | |||
| 402 | + | if err := sw.waiter.Pin(int32(pipeFD)); err != nil { | |||
| 403 | + | sw.log.Warn().Err(err).Msg("sync waiter") | |||
| 404 | + | sw.fail(nil) | |||
| 405 | + | return | |||
| 406 | + | } | |||
| 407 | for ctx.Err() == nil { | 407 | for ctx.Err() == nil { | ||
| 408 | live := waits[:0] | 408 | live := waits[:0] | ||
| 409 | for _, w := range waits { | 409 | for _, w := range waits { | ||
| @@ -468,6 +468,13 @@ func (sw *syncWaiter) run(ctx context.Context) { | |||||
| 468 | // close stops the waiter's input side. | 468 | // close stops the waiter's input side. | ||
| 469 | func (sw *syncWaiter) close() { sw.pipeW.Close() } | 469 | func (sw *syncWaiter) close() { sw.pipeW.Close() } | ||
| 470 | 470 | ||||
| 471 | + | // discard releases what run would have closed, for a waiter whose run | |||
| 472 | + | // never starts. | |||
| 473 | + | func (sw *syncWaiter) discard() { | |||
| 474 | + | sw.pipeR.Close() | |||
| 475 | + | sw.waiter.Close() | |||
| 476 | + | } | |||
| 477 | + | ||||
| 471 | // syncHold is a buffer's explicit-sync state while it is on screen: the | 478 | // syncHold is a buffer's explicit-sync state while it is on screen: the | ||
| 472 | // acquire fence handed to readers and the release point to signal. | 479 | // acquire fence handed to readers and the release point to signal. | ||
| 473 | type syncHold struct { | 480 | type syncHold struct { | ||
M internal/adapters/wayland/syncwait_test.go +2 -4
| @@ -19,8 +19,7 @@ func TestSyncWaitAlreadySignalled(t *testing.T) { | |||
| 19 | 19 | if err != nil { | |
| 20 | 20 | t.Fatal(err) | |
| 21 | 21 | } | |
| 22 | - | defer sw.pipeR.Close() | |
| 23 | - | defer sw.waiter.Close() | |
| 22 | + | defer sw.discard() | |
| 24 | 23 | defer sw.close() | |
| 25 | 24 | w, err := sw.watch(syncPoint{tl: &timeline{dev: dev, handle: 7}, point: 4}) | |
| 26 | 25 | if err != nil || !sw.fired(w) { | |
| @@ -34,8 +33,7 @@ func TestImplicitPollErrorRejectedAndWarnOnce(t *testing.T) { | |||
| 34 | 33 | if err != nil { | |
| 35 | 34 | t.Fatal(err) | |
| 36 | 35 | } | |
| 37 | - | defer sw.pipeR.Close() | |
| 38 | - | defer sw.waiter.Close() | |
| 36 | + | defer sw.discard() | |
| 39 | 37 | defer sw.close() | |
| 40 | 38 | var output bytes.Buffer | |
| 41 | 39 | sw.log = zerowrap.New(zerowrap.Config{Output: &output}) | |
| @@ -19,8 +19,7 @@ func TestSyncWaitAlreadySignalled(t *testing.T) { | |||||
| 19 | if err != nil { | 19 | if err != nil { | ||
| 20 | t.Fatal(err) | 20 | t.Fatal(err) | ||
| 21 | } | 21 | } | ||
| 22 | - | defer sw.pipeR.Close() | 22 | + | defer sw.discard() |
| 23 | - | defer sw.waiter.Close() | |||
| 24 | defer sw.close() | 23 | defer sw.close() | ||
| 25 | w, err := sw.watch(syncPoint{tl: &timeline{dev: dev, handle: 7}, point: 4}) | 24 | w, err := sw.watch(syncPoint{tl: &timeline{dev: dev, handle: 7}, point: 4}) | ||
| 26 | if err != nil || !sw.fired(w) { | 25 | if err != nil || !sw.fired(w) { | ||
| @@ -34,8 +33,7 @@ func TestImplicitPollErrorRejectedAndWarnOnce(t *testing.T) { | |||||
| 34 | if err != nil { | 33 | if err != nil { | ||
| 35 | t.Fatal(err) | 34 | t.Fatal(err) | ||
| 36 | } | 35 | } | ||
| 37 | - | defer sw.pipeR.Close() | 36 | + | defer sw.discard() |
| 38 | - | defer sw.waiter.Close() | |||
| 39 | defer sw.close() | 37 | defer sw.close() | ||
| 40 | var output bytes.Buffer | 38 | var output bytes.Buffer | ||
| 41 | sw.log = zerowrap.New(zerowrap.Config{Output: &output}) | 39 | sw.log = zerowrap.New(zerowrap.Config{Output: &output}) | ||