Bug 194370 – [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

WebProcessPool::platformInitialize unsafely calls g_unsetenv to unset WEBKIT_INSPECTOR_SERVER. WebKit must never modify environment variables in the UI process, because it could cause applications to crash. (The only safe place to modify environment variables is the very top of main() in secondary processes. It can't be done at all in the UI process.) So we need to find some way to avoid the need to call unsetenv here.

Also, note this function is duplicated between WebProcessPoolGtk.cpp and WebProcessPoolWPE.cpp, so both places need to be fixed, or the implementations shared.

[Carlos Garcia Campos]

I don't think there's anything unsafe in that.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #1)
> I don't think there's anything unsafe in that.

I think we've discussed this before, but for reference: it can crash if a secondary thread is calling getenv(), and getenv() is used everywhere. setenv/unsetenv is really, really dangerous. It's impossible to use safely in WebProcessPoolGtk.cpp because applications can create new WebKitWebContext objects after creating secondary threads.

[Michael Catanzaro]

Created attachment 361399 [details]
Patch

[Michael Catanzaro]

Created attachment 361400 [details]
Patch

[Darin Adler]

Comment on attachment 361400 [details]
Patch

While I’m not an expert on every aspect of these platforms, this patch seems to contain clearly correct changes

[Michael Catanzaro]

Carlos, I haven't tested remote inspector, but it looks like it should be fine. Does this need anything more?

For reference regarding unsafety, we have practical examples:

 * We have a public, documented example of unsetenv() usage bringing down gnome-session, resulting in full desktop crash. That was https://bugzilla.gnome.org/show_bug.cgi?id=754951.
 * Igalia had a project a few years ago where unsafe setenv() use caused a customer's devices to randomly fail to boot, resulting in problems.

As for theoretical documentation:

https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html documents setenv() as MT-Unsafe const:env

https://www.gnu.org/software/libc/manual/html_node/Conditionally-Safe-Features.html#Conditionally-Safe-Features documents the const annotation:

"""
Functions marked with const as an MT-Safety issue non-atomically modify internal objects that are better regarded as constant, because a substantial portion of the GNU C Library accesses them without synchronization. Unlike race, that causes both readers and writers of internal objects to be regarded as MT-Unsafe and AS-Unsafe, this mark is applied to writers only. 
"""

Then https://www.gnu.org/software/libc/manual/html_node/Other-Safety-Remarks.html#Other-Safety-Remarks documents the env annotation:

"""
Functions marked with env as an MT-Safety issue access the environment with getenv or similar, without any guards to ensure safety in the presence of concurrent modifications.

We do not mark these functions as MT- or AS-Unsafe, however, because functions that modify the environment are all marked with const:env and regarded as unsafe. Being unsafe, the latter are not to be called when multiple threads are running or asynchronous signals are enabled, and so the environment can be considered effectively constant in these contexts, which makes the former safe.
""" 

I also wrote a blog post about this problem last year: https://blogs.gnome.org/mcatanzaro/2018/02/28/on-setenv-and-explosions/

[Darin Adler]

And more importantly, there’s no need to unset the environment variable. I mean, if we tried and it didn’t work, we don’t need to "cover up the evidence" that it was ever tried.

[Darin Adler]

Sorry, I don’t mean "more importantly".

I mean that since it’s not needed, we don’t need to understand precisely exactly how bad an idea it is; we can just remove it if there’s any possible downside of keeping it at all.

[Michael Catanzaro]

Well unsetting this environment variable is done to stop the web process from trying to connect to the UI process server (which has failed to start for whatever reason). But I *think* there's indeed no particular reason to do that: I suspect it can just try and fail and that should probably be fine.

But if it *is* really needed, then we'll need a new IPC message to communicate that, rather than abusing the environment.

[WebKit Commit Bot]

Comment on attachment 361400 [details]
Patch

Rejecting attachment 361400 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 361400, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=361400&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=194370&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 361400 from bug 194370.
Fetching: https://bugs.webkit.org/attachment.cgi?id=361400
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 6 diffs from patch file(s).
patching file Source/JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp
patching file Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp
patching file Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp
Hunk #1 FAILED at 44.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp.rej
patching file Source/WebKit/UIProcess/wpe/WebProcessPoolWPE.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/11119949

[Michael Catanzaro]

Committed r241304: <https://trac.webkit.org/changeset/241304>