Skip to content

Comments

Fix of use-after-free error #211#212

Closed
ateska wants to merge 2 commits intoLibVNC:masterfrom
TeskaLabs:master
Closed

Fix of use-after-free error #211#212
ateska wants to merge 2 commits intoLibVNC:masterfrom
TeskaLabs:master

Conversation

@ateska
Copy link

@ateska ateska commented Dec 27, 2017

Fixes #211

@ateska ateska changed the title Fix of free-after-use error. Fix of use-after-free error #211 Dec 28, 2017
@bk138
Copy link
Member

bk138 commented Jun 16, 2018

Hi @ateska
were you running a multi-threaded server?

@Quentin01
Copy link
Contributor

Quentin01 commented Aug 9, 2018

The issue is happening on Virtualbox (see Virtualbox Ticket #17396).

Because Virtualbox is using a multi-threaded server with pthread enabled the use-after-free results in a wait to an non-existing condition-variable, leading to a freeze of the Virtual Machine in a buggy state.

This PR correctly fixes the issue even if its incomplete:

  • The lock is not really optional, we should add it (there are other parts of the code iterating on the clients and freeing them in a loop)
  • Because the clientConnectionGone is freeing the client and all its regions, the clientOutput thread is segfaulting when accessing cl->modifiedRegion.

I do have a fix for both and will provide a PR ASAP.

@ateska
Copy link
Author

ateska commented Aug 9, 2018

The issue happens on iOS platform when a server and client that runs in separated threads shutdowns simultaneously. It happens in 100% cases however - not the typical race condition.

@Quentin01
Copy link
Contributor

Quentin01 commented Aug 9, 2018

The issue is also 100% reproducible in Virtualbox. If you do have a client connected to the multithreaded-server during the call to rfbShutdownServer, the process will wait on the non-existing condition-variable forever.

Even if Virtualbox doesn't really crash directly in the first use-after-free.

@bk138
Copy link
Member

bk138 commented Jan 6, 2019

Closing this in favour of #238, thanks for spotting #211, @ateska!

@bk138 bk138 closed this Jan 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants