Hi,
Have you ever seen crashes in the CI system that ended up in
getenv? For example in
http://testresults.qt.io/ci/QtBase_5.4.2_Integration/build_00020/linux-g++_no-widgets_Ubuntu_12.04_x64/log.txt.gz
There are many more of these kinds of crashes throughout our
tests and whenever it happens we ignore the problem and just
press the "stage" button again. Must be some instability or bad
configuration on the CI machine, right?
It turns out, it isn't. Lars and I got to the bottom of this earlier
this week and the finding is a different one:
In shockingly many places in Qt itself as well as in our unit tests we access
environment variables and in some places we also change them.
The problem is that access to the environment is an operation that
is inherently unsafe from a threading perspective. There's a process
global "environ" variable, getenv() returns a bare pointer and
putenv/setenv modify that very environment. And there's no way around
that, the C run-time is not safe. Let one thread call getenv() - which iterates
through the environ array - and then let another thread call putenv/setenv
to modify the same array. The result is a seemingly random crash - or as it
turns
out: Frequent crashes in our CI system when trying to get changes into Qt :)
>From a practical perspective we are particularly vulnerable on Linux to this,
because QThread itself calls getenv() each time we start up a thread - it's
the can-we-use-glib-event-dispatcher-or-was-it-disabled check. Now we could
try to change qthread_unix.cpp to not do that, but let's be honest: That's a
drop in the ocean of places where we access (reading and writing!) the
environment in Qt. Just grep for putenv/setenv/getenv in qtbase/src
and be surprised.
There are various options about what we can do with different degrees of
"perfection",
but ultimately it's all going to require a compromise. The option that we are
favoring
at the moment is two-fold:
1) Policy in Qt is to use the Qt wrappers for accessing the environment
(qgetenv, etc.).
2) These functions we protect with a mutex.
Yes, this isn't perfect, because we still include large quantities of code in
src/3rdparty in Qt
that has unprotected calls to getenv(). But then that hasn't stopped us from
patching
that code in the past.
The concrete proposal of change is at
https://codereview.qt-project.org/#/c/111158/
What do you think?
I'd most appreciate a +2 or a concrete patch with an alternate solution.
Simon
_______________________________________________
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
