| Summary: | [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
| Component: | WebKitGTK | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bugs-noreply, cgarcia, commit-queue, darin, ews-watchlist, hi, joepeck, keith_miller, mark.lam, mcatanzaro, msaboff, saam, webkit-bug-importer | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Attachments: |
|
||||||||
|
Description
Michael Catanzaro
2019-02-06 18:54:06 PST
I don't think there's anything unsafe in that. (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. Created attachment 361399 [details]
Patch
Created attachment 361400 [details]
Patch
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
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/ 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. 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. 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. 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 commit-queue@webkit.org... 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 Committed r241304: <https://trac.webkit.org/changeset/241304> |