Thread safety issue with MemoryPool

[criezy]

I have had random crashes when enabling networking code for a while. Today I finally managed to get a backtrace for it, and it turns out this is due to MemoryPool not being thread safe and being accessed from two separate threads simultaneously.

Here is all the information I got so that it doesn't get lost:

First here is the reason of the crash:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x109261ca800)
    frame #0: 0x0000000101e89b69 scummvm`Common::MemoryPool::allocChunk(this=0x0000000104c07a70) at memorypool.cpp:105
   102 	
   103 		assert(_next);
   104 		void *result = _next;
-> 105 		_next = *(void **)result;
   106 		return result;
   107 	}
   108 	
  thread #10, name = 'SDLTimer', stop reason = EXC_BAD_ACCESS (code=1, address=0x109261ca800)
    frame #0: 0x0000000101e89b69 scummvm`Common::MemoryPool::allocChunk(this=0x0000000104c07a70) at memorypool.cpp:105
   102 	
   103 		assert(_next);
   104 		void *result = _next;
-> 105 		_next = *(void **)result;
   106 		return result;
   107 	}
   108 	

Here is the backtrace for thread 1:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x109261ca800)
  * frame #0: 0x0000000101e89b69 scummvm`Common::MemoryPool::allocChunk(this=0x0000000104c07a70) at memorypool.cpp:105
    frame #1: 0x0000000101e9572b scummvm`Common::String::incRefCount(this=0x00007fff5fbb8858) const at str.cpp:181
    frame #2: 0x0000000101e954de scummvm`Common::String::String(this=0x00007fff5fbb8880, str=0x00007fff5fbb8858) at str.cpp:85
    frame #3: 0x0000000101e9577d scummvm`Common::String::String(this=0x00007fff5fbb8880, str=0x00007fff5fbb8858) at str.cpp:78
    frame #4: 0x0000000101e98728 scummvm`Common::operator+(x=0x00007fff5fbb8858, y=".Visible") at str.cpp:742
    frame #5: 0x0000000101c1edb5 scummvm`GUI::Widget::isVisible(this=0x0000000104c79350) const at widget.cpp:208
    frame #6: 0x0000000101c1e72a scummvm`GUI::Widget::draw(this=0x0000000104c79350) at widget.cpp:102
    frame #7: 0x0000000101c1e97b scummvm`GUI::Widget::draw(this=0x0000000104c66e00) at widget.cpp:146
    frame #8: 0x0000000101c2f53f scummvm`GUI::TabWidget::draw(this=0x0000000104c66e00) at tab.cpp:332
    frame #9: 0x0000000101bbf82e scummvm`GUI::Dialog::drawWidgets(this=0x00007fff5fbb9278) at dialog.cpp:185
    frame #10: 0x0000000101bbf7cf scummvm`GUI::Dialog::drawDialog(this=0x00007fff5fbb9278, layerToDraw=kDrawLayerForeground) at dialog.cpp:173
    frame #11: 0x0000000101bce13d scummvm`GUI::GuiManager::redraw(this=0x0000000105087200) at gui-manager.cpp:253
    frame #12: 0x0000000101bcf54d scummvm`GUI::GuiManager::closeTopDialog(this=0x0000000105087200) at gui-manager.cpp:478
    frame #13: 0x0000000101bbf512 scummvm`GUI::Dialog::close(this=0x00007fff5fbb8dc8) at dialog.cpp:102
    frame #14: 0x0000000101c2a459 scummvm`GUI::PopUpDialog::handleMouseUp(this=0x00007fff5fbb8dc8, x=34, y=244, button=1, clickCount=1) at popup.cpp:182
    frame #15: 0x0000000101bcef88 scummvm`GUI::GuiManager::processEvent(this=0x0000000105087200, event=0x00007fff5fbb8d00, activeDialog=0x00007fff5fbb8dc8) at gui-manager.cpp:588
    frame #16: 0x0000000101bce72a scummvm`GUI::GuiManager::runLoop(this=0x0000000105087200) at gui-manager.cpp:359
    frame #17: 0x0000000101bbf3da scummvm`GUI::Dialog::runModal(this=0x00007fff5fbb8dc8) at dialog.cpp:80
    frame #18: 0x0000000101c2aee2 scummvm`GUI::PopUpWidget::handleMouseDown(this=0x0000000104c7aca0, x=84, y=10, button=1, clickCount=1) at popup.cpp:409
    frame #19: 0x0000000101bbf99a scummvm`GUI::Dialog::handleMouseDown(this=0x00007fff5fbb9278, x=225, y=53, button=1, clickCount=1) at dialog.cpp:205
    frame #20: 0x0000000101bcef36 scummvm`GUI::GuiManager::processEvent(this=0x0000000105087200, event=0x00007fff5fbb9040, activeDialog=0x00007fff5fbb9278) at gui-manager.cpp:583
    frame #21: 0x0000000101bce72a scummvm`GUI::GuiManager::runLoop(this=0x0000000105087200) at gui-manager.cpp:359
    frame #22: 0x0000000101bbf3da scummvm`GUI::Dialog::runModal(this=0x00007fff5fbb9278) at dialog.cpp:80
    frame #23: 0x0000000101bd4a53 scummvm`GUI::LauncherDialog::handleCommand(this=0x00007fff5fbb9988, sender=0x0000000104d5ce70, cmd=1330664526, data=0) at launcher.cpp:646
    frame #24: 0x0000000101c95ca5 scummvm`GUI::CommandSender::sendCommand(this=0x0000000104d5ce70, cmd=1330664526, data=0) at object.h:55
    frame #25: 0x0000000101c1fba1 scummvm`GUI::ButtonWidget::handleMouseUp(this=0x0000000104d5ce70, x=60, y=11, button=1, clickCount=1) at widget.cpp:332
    frame #26: 0x0000000101bbfb04 scummvm`GUI::Dialog::handleMouseUp(this=0x00007fff5fbb9988, x=569, y=292, button=1, clickCount=1) at dialog.cpp:226
    frame #27: 0x0000000101bcef88 scummvm`GUI::GuiManager::processEvent(this=0x0000000105087200, event=0x00007fff5fbb9900, activeDialog=0x00007fff5fbb9988) at gui-manager.cpp:588
    frame #28: 0x0000000101bce72a scummvm`GUI::GuiManager::runLoop(this=0x0000000105087200) at gui-manager.cpp:359
    frame #29: 0x0000000101bbf3da scummvm`GUI::Dialog::runModal(this=0x00007fff5fbb9988) at dialog.cpp:80
    frame #30: 0x000000010000a9b4 scummvm`launcherDialog() at main.cpp:106
    frame #31: 0x0000000100009cbe scummvm`::scummvm_main(argc=1, argv=0x00007fff5fbffb08) at main.cpp:501
    frame #32: 0x0000000100007af0 scummvm`main(argc=1, argv=0x00007fff5fbffb08) at macosx-main.cpp:45
    frame #33: 0x00007fff93fd3235 libdyld.dylib`start + 1
    frame #34: 0x00007fff93fd3235 libdyld.dylib`start + 1

And here is the backtrace for thread 10:

* thread #10, name = 'SDLTimer', stop reason = EXC_BAD_ACCESS (code=1, address=0x109261ca800)
  * frame #0: 0x0000000101e89b69 scummvm`Common::MemoryPool::allocChunk(this=0x0000000104c07a70) at memorypool.cpp:105
    frame #1: 0x0000000101e9572b scummvm`Common::String::incRefCount(this=0x000070000a3f6920) const at str.cpp:181
    frame #2: 0x0000000101e954de scummvm`Common::String::String(this=0x0000000114b0e1a0, str=0x000070000a3f6920) at str.cpp:85
    frame #3: 0x0000000101e9577d scummvm`Common::String::String(this=0x0000000114b0e1a0, str=0x000070000a3f6920) at str.cpp:78
    frame #4: 0x0000000101e825cd scummvm`Common::JSONValue::JSONValue(this=0x0000000114b0e220, stringValue=0x000070000a3f6920) at json.cpp:537
    frame #5: 0x0000000101e8216d scummvm`Common::JSONValue::JSONValue(this=0x0000000114b0e220, stringValue=0x000070000a3f6920) at json.cpp:535
    frame #6: 0x0000000101e7fba9 scummvm`Common::JSONValue::parse(data=0x000070000a3f8b20) at json.cpp:281
    frame #7: 0x0000000101e80aaf scummvm`Common::JSONValue::parse(data=0x000070000a3f8b20) at json.cpp:409
    frame #8: 0x0000000101e813f6 scummvm`Common::JSONValue::parse(data=0x000070000a3f8b20) at json.cpp:466
    frame #9: 0x0000000101e80aaf scummvm`Common::JSONValue::parse(data=0x000070000a3f8b20) at json.cpp:409
    frame #10: 0x0000000101e7f871 scummvm`Common::JSON::parse(data="}, {\".tag\": \"file\", \"name\": \"comi-fr.s14\", \"path_lower\": \"/saves/comi-fr.s14\", \"path_display\": \"/saves/comi-fr.s14\", \"id\": \"id:EW6r9glAsrAAAAAAAAAA5w\", \"client_modified\": \"2016-09-18T04:33:54Z\", \"server_modified\": \"2016-09-18T04:33:54Z\", \"rev\": \"e74da51434\", \"size\": 92075, \"content_hash\": \"62604193f7a654dee5a00f9a38e83b560dab2b1fd6b48cdb4db3a5c2daf7c58d\"}, {\".tag\": \"file\", \"name\": \"comi-fr.s13\", \"path_lower\": \"/saves/comi-fr.s13\", \"path_display\": \"/saves/comi-fr.s13\", \"id\": \"id:EW6r9glAsrAAAAAAAAAA6A\", \"client_modified\": \"2016-09-18T04:33:58Z\", \"server_modified\": \"2016-09-18T04:33:58Z\", \"rev\": \"e84da51434\", \"size\": 82812, \"content_hash\": \"63b98d2446e54eaa0ec8535d6eb72087bf85b70b17f7bfd8e235df80ea91737e\"}, {\".tag\": \"file\", \"name\": \"comi-fr.s12\", \"path_lower\": \"/saves/comi-fr.s12\", \"path_display\": \"/saves/comi-fr.s12\", \"id\": \"id:EW6r9glAsrAAAAAAAAAA6Q\", \"client_modified\": \"2016-09-18T04:34:02Z\", \"server_modified\": \"2016-09-18T04:34:02Z\", \"rev\": \"e94da51434\", \"size\": 78583, \"content_hash\": \"f30e111adee5d01b6c8ca"...) at json.cpp:82
    frame #11: 0x0000000101c8c54b scummvm`Networking::CurlJsonRequest::handle(this=0x000000010686d800) at curljsonrequest.cpp:75
    frame #12: 0x0000000101c88144 scummvm`Networking::ConnectionManager::interateRequests(this=0x0000000104d73e10) at connectionmanager.cpp:159
    frame #13: 0x0000000101c87f1e scummvm`Networking::ConnectionManager::handle(this=0x0000000104d73e10) at connectionmanager.cpp:135
    frame #14: 0x0000000101c87e99 scummvm`Networking::connectionsThread(ignored=0x0000000000000000) at connectionmanager.cpp:104
    frame #15: 0x0000000101c501b1 scummvm`DefaultTimerManager::handler(this=0x0000000104e57de0) at default-timer.cpp:105
    frame #16: 0x0000000101cc6edb scummvm`timer_handler(interval=10, param=0x0000000104e57de0) at sdl-timer.cpp:33
    frame #17: 0x000000010442afa9 libSDL2-2.0.0.dylib`SDL_TimerThread(_data=0x00000001044a8618) at SDL_timer.c:166 [opt]
    frame #18: 0x000000010442a94c libSDL2-2.0.0.dylib`SDL_RunThread(data=0x0000000104e52b30) at SDL_thread.c:283 [opt]
    frame #19: 0x0000000104484369 libSDL2-2.0.0.dylib`RunThread(data=<unavailable>) at SDL_systhread.c:74 [opt]
    frame #20: 0x00007fff941ec93b libsystem_pthread.dylib`_pthread_body + 180
    frame #21: 0x00007fff941ec887 libsystem_pthread.dylib`_pthread_start + 286
    frame #22: 0x00007fff941ec08d libsystem_pthread.dylib`thread_start + 13

[digitall]

​https://github.com/scummvm/scummvm/pull/1241

[criezy]

Fixed by merging PR #1241.