diff --git a/client/vnc/server/session_cursor.go b/client/vnc/server/session_cursor.go index cc059fb97..c791c70ce 100644 --- a/client/vnc/server/session_cursor.go +++ b/client/vnc/server/session_cursor.go @@ -11,13 +11,17 @@ import ( // when the client negotiated the encoding and the platform exposes a // cursor source whose serial has changed since the last emission. A nil // return means "do not include a cursor rect in this FramebufferUpdate". -func (s *session) pendingCursorRect() []byte { +// pf is the format the rest of this update is being encoded in, passed in +// rather than read again here: a SetPixelFormat landing mid-update would +// otherwise pack the cursor at the new shifts and the framebuffer at the old +// ones, and the client would paint a correctly coloured desktop under a cursor +// with its channels swapped. +func (s *session) pendingCursorRect(pf clientPixelFormat) []byte { s.encMu.RLock() supported := s.clientSupportsCursor failed := s.cursorSourceFailed composite := s.showRemoteCursor lastSerial := s.lastCursorSerial - pf := s.pf s.encMu.RUnlock() if !supported || failed || composite { return nil diff --git a/client/vnc/server/session_cursor_test.go b/client/vnc/server/session_cursor_test.go index 7bcc1dc89..a103ed555 100644 --- a/client/vnc/server/session_cursor_test.go +++ b/client/vnc/server/session_cursor_test.go @@ -47,13 +47,13 @@ func TestPendingCursorRect_SwitchingBackToALowerSerial(t *testing.T) { s := newCursorSession(src) // The arrow, then an I-beam the X server happens to number higher. - require.NotNil(t, s.pendingCursorRect(), "first cursor must be sent") + require.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat()), "first cursor must be sent") src.serial = 250 - require.NotNil(t, s.pendingCursorRect(), "a different cursor must be sent") + require.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat()), "a different cursor must be sent") // Back to the arrow: a lower serial, and still a real change. src.serial = 100 - assert.NotNil(t, s.pendingCursorRect(), "returning to an earlier cursor must be sent, not dropped as stale") + assert.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat()), "returning to an earlier cursor must be sent, not dropped as stale") } // An unchanged serial is still the one case that must not produce a rect, @@ -63,6 +63,6 @@ func TestPendingCursorRect_UnchangedSerialIsSkipped(t *testing.T) { src := &stubCursorSource{img: sprite, serial: 7} s := newCursorSession(src) - require.NotNil(t, s.pendingCursorRect()) - assert.Nil(t, s.pendingCursorRect(), "the same cursor must not be re-sent") + require.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat())) + assert.Nil(t, s.pendingCursorRect(defaultClientPixelFormat()), "the same cursor must not be re-sent") } diff --git a/client/vnc/server/session_encode.go b/client/vnc/server/session_encode.go index 56d1b5ebe..c00cafcc8 100644 --- a/client/vnc/server/session_encode.go +++ b/client/vnc/server/session_encode.go @@ -440,7 +440,11 @@ func (s *session) swapPrevCur() { // pseudo-rect into the same message so a shape change (e.g. hovering onto // a resize handle) reaches the client without waiting for a dirty frame. func (s *session) sendEmptyUpdate() error { - cursorRect := s.pendingCursorRect() + s.encMu.RLock() + pf := s.pf + s.encMu.RUnlock() + + cursorRect := s.pendingCursorRect(pf) if cursorRect == nil { var buf [4]byte buf[0] = serverFramebufferUpdate @@ -464,7 +468,7 @@ func (s *session) sendFullUpdate(img *image.RGBA) error { zlib := s.zlib s.encMu.RUnlock() - cursorRect := s.pendingCursorRect() + cursorRect := s.pendingCursorRect(pf) rectCount := uint16(1) if cursorRect != nil { rectCount++ @@ -534,7 +538,14 @@ func (s *session) writeFramed(buf []byte) error { // their source tiles are read from the client's pre-update framebuffer state, // before any subsequent rect overwrites them. func (s *session) sendDirtyAndMoves(img *image.RGBA, moves []copyRectMove, rects [][4]int) error { - cursorRect := s.pendingCursorRect() + // One snapshot for the whole message: every tile below and the cursor rect + // are packed at these shifts, so a SetPixelFormat arriving mid-update + // cannot leave one rectangle in a different format from its neighbours. + s.encMu.RLock() + pf := s.pf + s.encMu.RUnlock() + + cursorRect := s.pendingCursorRect(pf) if len(moves) == 0 && len(rects) == 0 && cursorRect == nil { return nil } @@ -570,7 +581,7 @@ func (s *session) sendDirtyAndMoves(img *image.RGBA, moves []copyRectMove, rects for _, r := range rects { x, y, w, h := r[0], r[1], r[2], r[3] - rectBuf := s.encodeTile(img, x, y, w, h) + rectBuf := s.encodeTile(img, x, y, w, h, pf) if _, err := s.conn.Write(rectBuf); err != nil { return err } @@ -587,9 +598,10 @@ func (s *session) sendDirtyAndMoves(img *image.RGBA, moves []copyRectMove, rects // // Output omits the 4-byte FramebufferUpdate header; callers combine multiple // tiles into one message. -func (s *session) encodeTile(img *image.RGBA, x, y, w, h int) []byte { +// pf is the caller's snapshot, so every tile of one update packs at the same +// shifts even if the client renegotiates the format while it is being built. +func (s *session) encodeTile(img *image.RGBA, x, y, w, h int, pf clientPixelFormat) []byte { s.encMu.RLock() - pf := s.pf useHextile := s.useHextile useTight := s.useTight tight := s.tight