Page MenuHomeFreeBSD

Make RFB_ENCODING_ZLIB message handling idempotent.
ClosedPublic

Authored by markj on Jun 17 2019, 3:33 PM.

Details

Summary

The VNC server uses two threads to manage a connection: one thread sends
periodic updates to the client, and another handles client requests.
For the most part they are unsynchonized, and we may be simultaneously
handling a request and pushing an update.

Fix two bugs in the RFB_ENCODING_ZLIB message handler:

  • Ensure that we don't reinitialize the zlib stream if multiple messages are received.
  • Don't indicate to the sending thread that zlib compression is to be used until we've finished initializing the stream.

This is not a complete fix but I wanted to have something small that can
easily be MFCed. In particular, it is insufficient on platforms where
stores can be reordered (which I guess is relevant since there is an
arm64 port).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Jun 17 2019, 3:33 PM
markj added a reviewer: cem.Jun 17 2019, 3:34 PM
cem accepted this revision.Jun 17 2019, 3:39 PM

As a first-cut workaround, LGTM.

Longer term, I don't like that there's no real synchronization between the two threads. Also I think this could pretty easily be converted to a single thread, obviating the need for synchronization.

This revision is now accepted and ready to land.Jun 17 2019, 3:39 PM
markj added a comment.Jun 17 2019, 3:46 PM
In D20673#446746, @cem wrote:

As a first-cut workaround, LGTM.
Longer term, I don't like that there's no real synchronization between the two threads. Also I think this could pretty easily be converted to a single thread, obviating the need for synchronization.

Agreed. I'll try to find out why it was written this way.

rgrimes accepted this revision as: rgrimes.Jun 19 2019, 2:01 PM

In agreement on it probably needs cleaned up, but first finding out why it was done this way as a safe position to take.

This revision was automatically updated to reflect the committed changes.