-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[core] Fix initialization race on util::mapbox::isMapboxURL #6455
Conversation
This function can be called from different threads and was depending on a global `std::string protocol` which can do heap allocations. The first thread to use it would initialized it which is not exactly safe, so this patch moves it to a `char *` that is just a pointer to the data segment, so no races. Valgrind detected it on #6431. ``` ==9874== Conditional jump or move depends on uninitialised value(s) ==9874== at 0x4C307C2: __memcmp_sse4_1 (in /home/travis/build/mapbox/mapbox-gl-native/mason_packages/linux-x86_64/valgrind/latest/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==9874== by 0x79896E: mbgl::util::mapbox::isMapboxURL(std::string const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x6DC756: mbgl::style::TileSourceImpl::parseTileJSON(std::string const&, std::string const&, mbgl::SourceType, unsigned short) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x6DDA6A: std::_Function_handler<void (mbgl::Response), mbgl::style::TileSourceImpl::loadDescription(mbgl::FileSource&)::{lambda(mbgl::Response)#1}>::_M_invoke(std::_Any_data const&, mbgl::Response&&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x50BE80: std::_Function_handler<void (), mbgl::StubFileSource::StubFileSource()::{lambda()#1}>::_M_invoke(std::_Any_data const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x70972A5: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x70A3621: QTimer::timerEvent(QTimerEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x7098053: QObject::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x60CCC8B: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.2.1) ==9874== by 0x60D1E55: QApplication::notify(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.2.1) ==9874== by 0x706FC2C: QCoreApplication::notifyInternal(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x70BC1AC: QTimerInfoList::activateTimers() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== ```
@tmpsantos, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @tmcw and @yhahn to be potential reviewers. |
/cc @1ec5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it all makes sense now!
This fix is fine, but for future reference, a good pattern to use if you need a thread-safe, static, non-primitive object, and you can't use a
As of C++11, initialization of local statics is guaranteed to be thread-safe. |
Oh, did not know about this one. Thanks! |
This function can be called from different threads and was depending on a global
std::string protocol
which can do heap allocations.The first thread to use it would initialized it which is not exactly safe, so this patch moves it to a
char *
that is just a pointer tothe data segment, so no races.
Valgrind detected it on #6431.