Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix concurrent access to String MemoryPool instance #1241

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@criezy
Copy link
Member

commented Jul 7, 2018

The Common::String class uses a global MemoryPool instance to allocate and deallocate memory. However when Common::String objects are used simultaneously in different threads this results in concurrent access to this MemoryPool and a crash. This notably happens when enabling the Cloud feature.

This PR adds a mutex to protect the access to the MemoryPool. However mutexes are only available once the g_system instance has been set and the backend initialized and only until it is deleted. However Strings are used both too early (for example in the OSystem_POSIX constructor) and too late (for example in the OSystem_SDL destructor when it deletes the logger after deleting the mutex manager). So I had to do more changes than I wanted too to handle those cases.

In the end I am not too happy with what I had to do, but it works (or at least it seems to). Without this change I have a ~50% chance of a random crash when opening and closing the Options dialog just after starting ScummVM when it synchronises the saves with the cloud. I have not been able to reproduce it after making those changes.

This fixes Bug #10524.

criezy added some commits Jul 6, 2018

OSYSTEM: Add backendInitialized() function
Some feature, such as mutexes, are only available once the backend
has been initialized. This new function can be used to avoid using
those feature too early or too late.
COMMON: Add mutex to protect access to the String memory pool
This fixes a crash due to concurrent access to the global MemoryPool
used by the String class when String objects are used simultaneously
from several threads (as is for example the case when enabling the
cloud features).

See bug #10524: Thread safety issue with MemoryPool
@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

Overall, the changes look good. Well done! :)

@digitall

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

OK to merge?

@criezy

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

Possibly, depending on what you think about the other options below.

I was waiting until I had the time to investigate two possible alternatives, but I did not have time to do it so far, and I don't know if they are viable. The alternatives I had in mind are:

  • Make the MemoryPool class itself thread-safe, which would reuse a big chunk of this PR but use the mutex inside the class instead of around the usage of the class in String. But I suspect in most places where MemoryPool is used we don't need thread-safety, and might not want the added cost of using a mutex.
  • Remove the use of MemoryPool from String altogether. I am unsure of the performance impact though, but I think it would be quite small. If we assume the same String is not used in two different threads we could for example use a simple malloc/free (or new/delete). This would be a lot simpler than the changes I proposed in this PR. But if we need to properly handle the possibility that the same String (or copies of it) might be used in multiple threads then any solution would likely suffer from the same issue than we have with MemoryPool, so we might as well continue using MemoryPool and merge this PR.

Note: in theory the U32String could suffer from the same issue since it also uses MemoryPool. I don't think U32String is currently used in any places that requires thread-safety, but we might want to consider doing the same change there in case it does in the future.

@digitall

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Hmm... That is a tricky judgement call, though any of the solutions would be better than a crash from concurrent access corruption of data :/

I think the best way might be the "Make the MemoryPool class itself thread-safe" option, but add a constructor parameter to the MemoryPool to skip the mutex usage i.e. bool threadSafe = false or similar ... Whether that defaults to ThreadSafe or SingleThreadOnly is another question i.e. safety vs. performance.

@criezy

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

Thinking a bit more about it, there are several issues with moving the use of the mutex inside the MemoryPool class:

  • Creation of the global String MemoryPool would no longer be protected by the mutex. This is not really an issue though as it is created very early, before we can even use mutexes and threads.
  • We need to destroy all the mutexes before the System instance is destroyed, and if we have a mutex in the MemoryPool class I am not sure how to handle that. Unless we use a single global mutex shared by all the MemoryPool that want to use a mutex. But that is not optimal.
  • If we want String to be properly thread-safe, including the possibility to use copies of the same String in multiple thread, then we also need to protect access to the _extern._refCount variable (not just its allocation/deallocation, but also assigning a value and increment and decrement). I just noticed that I have actually not done so. And if we want to do it we need to keep a mutex in String itself.

So I would be tempted to:

  • either go with the simpler option of dropping the use of MemoryPool in String
  • go with this PR with some small modification to try to protect access to the _extern._refCount and not just the MemoryPool. Here again optimally we would want to add a mutex in the _extern struct (and have separate mutexes for separate _refCount pointers), but we would have the issue of destroying them all when the System instance gets destroyed. So the simple option is to use the global mutex added in String by this PR for both access to the MemoryPool and other accesses to the _recCount.
@digitall

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

I think I would suggest your second option then i.e. add the modifications to protect refCount etc. and merge. If this does cause further issues, we can remove the MemoryPool from String as a fallback... hard to judge the perfect solution here as String is a very general and widely used class.

@criezy

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2018

After taking another look at this today I decided to push as is. If the need arises we can extend the use of mutexes later.

@criezy criezy closed this Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.