Bug 192405

Something will always be calling raw getenv() in secondary threads. Any safety will be an illusion at best.

(In reply to Michael Catanzaro from comment #1) > Something will always be calling raw getenv() in secondary threads. Any > safety will be an illusion at best. Well either way it would be nice to have a way to get back values in a nice way and avoid parsing strings. https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/network/curl/CurlContext.cpp#L50-L72 Also there's inconsistency with the use of g_getenv and getenv.

Accounting to the glibc document, it is safe to call getenv in multiple threads, but not safe with other libc. https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html > This function returns a string that is the value of the > environment variable name. You must not modify this string. In > some non-Unix systems not using the GNU C Library, it might be > overwritten by subsequent calls to getenv (but not by any other > library function). If the environment variable name is not > defined, the value is a null pointer. glib document also mentions the next g_getenv call may overrite the previous buffer. https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-getenv > the value of the environment variable, or NULL if the environment > variable is not found. The returned string may be overwritten by > the next call to g_getenv(), g_setenv() or g_unsetenv(). Following links are also interesting. 0000188: thread-safe getenv() - Austin Group Defect Tracker http://austingroupbugs.net/view.php?id=188 Bug 659326 – implement g_app_launch_context_get_environment for win32 https://bugzilla.gnome.org/show_bug.cgi?id=659326 I think we shouldn't assume glibc's behavior. We should avoid using getenv in WebKit.

getenv, getenv_s - cppreference.com https://en.cppreference.com/w/c/program/getenv > This function is not required to be thread-safe. Another call to > getenv, as well as a call to the POSIX functions setenv(), > unsetenv(), and putenv() may invalidate the pointer returned by a > previous call or modify the string obtained from a previous call. I didn't know getenv_s!

(In reply to Fujii Hironori from comment #3) > I think we shouldn't assume glibc's behavior. > We should avoid using getenv in WebKit. Yeah I think you're right, at least for cross-platform code. This is easier said than done, since no doubt loads of our dependencies use gettext or other things that use getenv(), and probably in threads. :( For WPE/GTK it would be totally infeasible and we will have to rely on glibc's safety assumptions. :/

Created attachment 361353 [details] Proof of concept

Attached a proof of concept (i.e. WTF and JSC only) of a WTF::Environment, based on discussion with Don. Let me know if any course corrections should be made. If we like this approach, then I can carry it out through the rest of Source/ and Tools/.

Created attachment 361357 [details] Proof of concept

Comment on attachment 361357 [details] Proof of concept View in context: https://bugs.webkit.org/attachment.cgi?id=361357&action=review I like Environment::get. Environment::set, ::add, and ::remove are all extremely dangerous, but I'm not opposed to adding them, as long as we make them hard to misuse. They should somehow ASSERT that WTF::initializeThreading has not yet been called. Even that is imperfect -- since threads might have been created outside WTF -- but that should avoid most potential misuse. Grepping Source/ for usage of setenv and unsetenv, I see mostly just bugs. There is unsafe use in DatabaseManagerCocoa.mm, in WebKitLegacy's WebView.mm, in WebProcessPoolGtk.cpp and WebProcessPoolWPE.cpp, and -- not sure -- but possibly also in PluginProcessMac.mm and AuxiliaryProcessMac.mm. So if developers try to use our safer versions that ASSERT() against misuse, that could be beneficial. (Of course, developers will still use [un]setenv directly, but we could write a style checker rule to prevent that.) > Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:78 > - [](GObject*, GAsyncResult* result, gpointer userData) { > + [&](GObject*, GAsyncResult* result, gpointer userData) { Looks like stack use after scope, right? You have captured inspectorServerString by reference, but it's a local variable in the parent scope, so it will be disaster when the lambda is called. Got to be really careful about capture by reference. We also prefer to explicitly capture just the variables you need: [inspectorServerString](GObject* ...) { > Source/WTF/wtf/PlatformWPE.cmake:19 > + glib/EnvironmentGlib.cpp I know existing files are inconsistent, but when adding new ones, let's use GLib with a capital L. Eventually I'll get bored and correct older files. But honestly, the GLib implementations seem low-value. We could probably just rename EnvironmentGeneric.cpp to Environment.cpp and use that. Looking at the implementations, g_getenv() just calls getenv() and g_setenv() just calls setenv(). (g_setenv() also contains a workaround for ancient systems that don't have setenv(), including a comment that points out it is leaking memory. We don't care.) So I'd remove the GLib implementation and use the generic one always. > Source/WTF/wtf/glib/EnvironmentGlib.cpp:40 > + g_setenv(variable.utf8().data(), value.utf8().data(), 1); But if we were to keep the GLib implementation, we should pass TRUE for the third parameter, since it is a gboolean, unlike setenv. And FALSE down below in the add function. (But let's just remove it.) > Source/WTF/wtf/glib/EnvironmentGlib.cpp:46 > +void add(const String& variable, const String& value) > +{ > + g_setenv(variable.utf8().data(), value.utf8().data(), 0); > +} Problem is the name add() doesn't adequately represent what this function does. It means "set the environment variable if it does not already exist, and do nothing otherwise." What would be a better name to express that?

Could also merge the WebKit-layer EnvironmentUtilities.cpp into this, to ensure that dangerous function is not misused either.

(In reply to Michael Catanzaro from comment #11) > in WebProcessPoolGtk.cpp and WebProcessPoolWPE.cpp Reported bug #194370

Thanks for the thorough review! (In reply to Michael Catanzaro from comment #11) > I like Environment::get. > > Environment::set, ::add, and ::remove are all extremely dangerous, but I'm > not opposed to adding them, as long as we make them hard to misuse. They > should somehow ASSERT that WTF::initializeThreading has not yet been called. > Even that is imperfect -- since threads might have been created outside WTF > -- but that should avoid most potential misuse. > > Grepping Source/ for usage of setenv and unsetenv, I see mostly just bugs. > There is unsafe use in DatabaseManagerCocoa.mm, in WebKitLegacy's > WebView.mm, in WebProcessPoolGtk.cpp and WebProcessPoolWPE.cpp, and -- not > sure -- but possibly also in PluginProcessMac.mm and AuxiliaryProcessMac.mm. > So if developers try to use our safer versions that ASSERT() against misuse, > that could be beneficial. (Of course, developers will still use [un]setenv > directly, but we could write a style checker rule to prevent that.) Cool, I'll see what I can do! > > Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:78 > > - [](GObject*, GAsyncResult* result, gpointer userData) { > > + [&](GObject*, GAsyncResult* result, gpointer userData) { > > Looks like stack use after scope, right? You have captured > inspectorServerString by reference, but it's a local variable in the parent > scope, so it will be disaster when the lambda is called. Got to be really > careful about capture by reference. We also prefer to explicitly capture > just the variables you need: > > [inspectorServerString](GObject* ...) { Derp, this was totally "last thing before I run to dinner" code... > ... I'd remove the GLib implementation and use the generic one always. Sounds good to me! > > Source/WTF/wtf/glib/EnvironmentGlib.cpp:46 > > +void add(const String& variable, const String& value) > > +{ > > + g_setenv(variable.utf8().data(), value.utf8().data(), 0); > > +} > > Problem is the name add() doesn't adequately represent what this function > does. It means "set the environment variable if it does not already exist, > and do nothing otherwise." What would be a better name to express that? I actually stole this name from HashMap because I couldn't think of a better one: https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/HashMap.h#L118-L128 Something like "setIfEmpty" could also work though.

Wowsers, it was not at all obvious to me at a glance, but Windows doesn't have setenv or unsetenv! That said, apparently the only Windows code (outside of Source/ThirdParty) that's modifying env vars is DLLLauncher (by means of SetEnvironmentVariable). So I suppose we could just #if out set/add/remove on Windows for now?

Yeah. Probably best to remove the the functions entirely, not just their implementations, to ensure nobody can call them except inside #!OS(WINDOWS) guards.

Created attachment 361478 [details] Checkpoint for EWS

Created attachment 361574 [details] Checkpoint for EWS

Created attachment 361592 [details] Checkpoint for EWS

Created attachment 361594 [details] Checkpoint for EWS

Created attachment 361595 [details] Checkpoint for EWS

Created attachment 361600 [details] All but the ASSERT

Created attachment 361613 [details] All but the ASSERT

Created attachment 361618 [details] All but the ASSERT

Created attachment 361653 [details] Patch

Attachment 361653 [details] did not pass style-queue: ERROR: Source/WTF/wtf/posix/EnvironmentPOSIX.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/win/EnvironmentWin.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/DataLog.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 88 files If any of these errors are false positives, please file a bug against check-webkit-style.

A couple of notes: 1. The refactor of TestController::libraryPathForTesting() might seem a bit more than necessary, but I was noticing locally that the doubling of String::fromUTF8 seemed to be causing failures. (In reply to Michael Catanzaro from comment #12) > Could also merge the WebKit-layer EnvironmentUtilities.cpp into this, to > ensure that dangerous function is not misused either. 2. I'd prefer to postpone this one to a later patch, since the API test itself would hit the ASSERT. :( 3. Here's hoping my estimation of which g_[un]setenv calls would hit the ASSERT is correct (in particular, in WebKitGLib/TestAutomationSession, I think the beforeAll() seems safe but the afterAll() probably isn't).

(In reply to Ross Kirsling from comment #42) > 2. I'd prefer to postpone this one to a later patch, since the API test > itself would hit the ASSERT. :( So that means the API test is wrong (at risk of randomly crashing). setenv() is *really* tricky! (See bug #194370.)

(In reply to Michael Catanzaro from comment #43) > (In reply to Ross Kirsling from comment #42) > > 2. I'd prefer to postpone this one to a later patch, since the API test > > itself would hit the ASSERT. :( > > So that means the API test is wrong (at risk of randomly crashing). If it's wrong, I'm not sure what's right though... It is consistent that WTF::initializeThreading() has already been called before the test even begins. > setenv() is *really* tricky! (See bug #194370.) I did base the WebProcessPool changes here on your patch over there. :)

(In reply to Ross Kirsling from comment #44) > If it's wrong, I'm not sure what's right though... It is consistent that > WTF::initializeThreading() has already been called before the test even > begins. So if you want to test [un]setenv, you know you have to do that before threads are initialized. Right? I'm not sure how you plan to test it.

(In reply to Michael Catanzaro from comment #45) > (In reply to Ross Kirsling from comment #44) > > If it's wrong, I'm not sure what's right though... It is consistent that > > WTF::initializeThreading() has already been called before the test even > > begins. > > So if you want to test [un]setenv, you know you have to do that before > threads are initialized. Right? I'm not sure how you plan to test it. I don't know either -- perhaps that function ought to just do an immutable string manipulation without editing the env vars itself. I'm happy to create a new bug for redoing EnvironmentUtilities, but I figure that this patch is large enough as it is, hehe.

Chris on my team pointed out that it's kind of unfortunate that we can't use `if (auto ...` and have to use !! everywhere due to Environment::get's String return type, and that Optional<String> would make life easier. I originally felt like Optional would be unnecessary wrapping given that a String may have a null impl, but in the end, I think it's better for usability so I'm going to switch.

Created attachment 361736 [details] Patch

Created attachment 361759 [details] Patch

Created attachment 361765 [details] Patch

Created attachment 361813 [details] Patch

Created attachment 361840 [details] Patch

This should hopefully be ready now.

Comment on attachment 361840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361840&action=review Nice! I bet it will break GTK API tests in debug, but if so, that's just exposing preexisting problems that can be handled separately. > Source/WTF/wtf/Environment.h:76 > +WTF_EXPORT_PRIVATE void setIfUndefined(const String& variable, const String& value); How about setIfNotDefined? > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:429 > - const char* tmpDir = getenv("TMPDIR"); > > + auto tmpDir = Environment::getRaw("TMPDIR"); Any reason you're changing char* to auto like this? It just makes it harder to read; now we have to look int Environment.h to see it returns a const char*. I usually reserve auto for when there's a cast on the rhs or the type is long. At least use auto* so we can tell it's a pointer. > Source/WTF/wtf/win/EnvironmentWin.cpp:55 > +void setIfUndefined(const String& variable, const String& value) > +{ > + if (get(variable)) I'd do the ASSERT here too, to ensure it crashes even if the variable is already defined. No problem to have an extra assert since it will get compiled out in release builds.

(In reply to Michael Catanzaro from comment #62) > > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:429 > > - const char* tmpDir = getenv("TMPDIR"); > > > > + auto tmpDir = Environment::getRaw("TMPDIR"); > > Any reason you're changing char* to auto like this? It just makes it harder > to read; now we have to look int Environment.h to see it returns a const > char*. I usually reserve auto for when there's a cast on the rhs or the type > is long. > > At least use auto* so we can tell it's a pointer. I was just following the strategy (from, say, Effective Modern C++) of preferring auto for locals in general, though WK certainly has a mix of both practices and doesn't specify either in its style guide. I totally agree that dropping the * isn't ideal though, and actually the vast majority of these could be explicitly const.

Created attachment 361933 [details] Patch

(In reply to Ross Kirsling from comment #63) > I was just following the strategy (from, say, Effective Modern C++) of > preferring auto for locals in general, though WK certainly has a mix of both > practices and doesn't specify either in its style guide. I totally agree > that dropping the * isn't ideal though, and actually the vast majority of > these could be explicitly const. Went with `const char*` instead of `const auto*` after all -- I don't feel too strongly either way, but my teammate Steph pointed out that (1) Effective Modern C++ would probably also recommend against const char* where avoidable and (2) unlike `int`, `char` would be hard to replace with something else and have every callsite "just work".

Created attachment 361937 [details] Patch

(In reply to Ross Kirsling from comment #66) > (2) unlike `int`, `char` would be hard to replace with > something else and have every callsite "just work". Hm yes, auto is often safer for integer types, that's true.

Also created bug #194612.

Created attachment 361942 [details] Patch

WinCairo EWS failure here is a fluke...

Comment on attachment 361942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361942&action=review Looks good, r=me. I still think you're overusing auto (one of the rare cases I will disagree with Scott Meyers ;) but we don't have a style rule about that currently, and this is hardly uncommon in WebKit, and the callsites are otherwise significantly easier to read now anyway, thanks to your convenience functions. > Tools/WebKitTestRunner/InjectedBundle/gtk/InjectedBundleGtk.cpp:48 > - if (!g_getenv("WEBKIT_TOP_LEVEL")) > - g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE); > + g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE); Well that doesn't look finished. It should use Environment::set, right? If it asserts, we can try to fix it in a followup (just needs moved to the top of web process main).

(In reply to Michael Catanzaro from comment #74) > Comment on attachment 361942 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361942&action=review > > Looks good, r=me. Thanks a bunch! > I still think you're overusing auto (one of the rare cases I will disagree > with Scott Meyers ;) but we don't have a style rule about that currently, > and this is hardly uncommon in WebKit, and the callsites are otherwise > significantly easier to read now anyway, thanks to your convenience > functions. One way or another, I too would love to have a clear style rule. :) Perhaps I should send a question to webkit-dev. > > Tools/WebKitTestRunner/InjectedBundle/gtk/InjectedBundleGtk.cpp:48 > > - if (!g_getenv("WEBKIT_TOP_LEVEL")) > > - g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE); > > + g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE); > > Well that doesn't look finished. It should use Environment::set, right? If > it asserts, we can try to fix it in a followup (just needs moved to the top > of web process main). Basically Mac was asserting from InjectedBundle::platformInitialize so I reverted changes to it for all platforms (after the last patch labelled "All but the ASSERT"), it's just that GTK+ also had a redundant conditional, so I figured I'd still delete it, haha. I can surely make this Environment::set right now, I just didn't want to tank your test bot. :(

How about moving it to the top of web process main?

Created attachment 362038 [details] Patch for landing

Created attachment 362040 [details] Patch for landing

Comment on attachment 362040 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=362040&action=review > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:49 > + Environment::setIfNotDefined("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR); Ah whoops, my bad. That's from WebKitTestRunner's InjectedBundle... we're moving this from Tools/ to Source/ :O Best just leave this for a follow-up bug.

(In reply to Michael Catanzaro from comment #79) > Comment on attachment 362040 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362040&action=review > > > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:49 > > + Environment::setIfNotDefined("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR); > > Ah whoops, my bad. That's from WebKitTestRunner's InjectedBundle... we're > moving this from Tools/ to Source/ :O > > Best just leave this for a follow-up bug. Oops! Alrighty then. 😅

Created attachment 362041 [details] Patch for landing

Comment on attachment 362041 [details] Patch for landing Clearing flags on attachment: 362041 Committed r241559: <https://trac.webkit.org/changeset/241559>

All reviewed patches have been landed. Closing bug.

<rdar://problem/48082754>

(In reply to Fujii Hironori from comment #3) > Accounting to the glibc document, it is safe to call getenv in multiple > threads, but not safe with other libc. Forgot about this (from way up at the top of the bug). Fujii is right, even getenv() isn't necessarily safe. But removing use of getenv() would be very difficult, probably not practical.

(In reply to Michael Catanzaro from comment #85) > Forgot about this (from way up at the top of the bug). Fujii is right, even > getenv() isn't necessarily safe. But removing use of getenv() would be very > difficult, probably not practical. If it will become a real problem, we can use getenv_s.

(In reply to WebKit Commit Bot from comment #82) > Committed r241559: <https://trac.webkit.org/changeset/241559> I'm seeing crashes when running layout tests with GuardMalloc on Mojave after this change: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit 0x0000000101f4f26d WebKit::EnvironmentUtilities::stripValuesEndingWithString(char const*, char const*) + 196 1 com.apple.WebKit 0x0000000101f50ce2 NetworkServiceInitializer + 38 2 com.apple.WebKit 0x00000001020af8a6 invocation function for block in WebKit::XPCEventHandler(NSObject<OS_xpc_object>*, WebKit::AuxiliaryProcessType) + 293 3 libxpc.dylib 0x00007fff616cc044 _xpc_connection_call_event_handler + 56 4 libxpc.dylib 0x00007fff616c9fda _xpc_connection_mach_event + 933 5 libdispatch.dylib 0x00007fff6144a6dd _dispatch_client_callout4 + 9 6 libdispatch.dylib 0x00007fff6145f0d6 _dispatch_mach_msg_invoke + 436 7 libdispatch.dylib 0x00007fff61450792 _dispatch_lane_serial_drain + 268 8 libdispatch.dylib 0x00007fff6145fc19 _dispatch_mach_invoke + 481 9 libdispatch.dylib 0x00007fff6145554b _dispatch_main_queue_callback_4CF + 813 10 com.apple.CoreFoundation 0x00007fff34a3d227 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9 11 com.apple.CoreFoundation 0x00007fff34a3c951 __CFRunLoopRun + 2289 12 com.apple.CoreFoundation 0x00007fff34a3be0e CFRunLoopRunSpecific + 455 13 com.apple.Foundation 0x00007fff36d57a9f -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280 14 com.apple.Foundation 0x00007fff36d57974 -[NSRunLoop(NSRunLoop) run] + 76 15 libxpc.dylib 0x00007fff616d01d7 _xpc_objc_main + 552 16 libxpc.dylib 0x00007fff616cfcd9 xpc_main + 433 17 com.apple.WebKit 0x00000001020afa0d WebKit::XPCServiceMain(int, char const**) + 105 18 libdyld.dylib 0x00007fff614973d5 start + 1

(In reply to Ryan Haddad from comment #87) > (In reply to WebKit Commit Bot from comment #82) > > Committed r241559: <https://trac.webkit.org/changeset/241559> > > I'm seeing crashes when running layout tests with GuardMalloc on Mojave > after this change: This seems to be 100% reproducible and blocks testing with GuardMalloc. I'm going to roll this out.

Re-opened since this is blocked by bug 194710

(In reply to Ryan Haddad from comment #87) > (In reply to WebKit Commit Bot from comment #82) > > Committed r241559: <https://trac.webkit.org/changeset/241559> > > I'm seeing crashes when running layout tests with GuardMalloc on Mojave > after this change: > > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.WebKit 0x0000000101f4f26d > WebKit::EnvironmentUtilities::stripValuesEndingWithString(char const*, char > const*) + 196 Strange, it looks like the commit left that function unchanged (broken, for follow-up in bug #194612) specifically to avoid surfacing crashes on the debug bots. Since we didn't change it, it's not clear why it's only crashing now. Anyway, guess we need to deal with it now.

What is GuardMalloc and how would I test with it on a Mojave machine? Not sure why getRaw would cause a problem, but then there was also a heisenbug in WKTR TestController (somehow related to CFString conversion) that went away by refactoring the callsites. One way or another, if rewriting EnvironmentUtilities::stripValuesEndingWithString is next on my list anyway, I suppose it makes sense to just drop it from this patch entirely.

Created attachment 362153 [details] Patch for landing

(In reply to Ross Kirsling from comment #91) > What is GuardMalloc and how would I test with it on a Mojave machine? It is a testing mode to find memory access bugs. I encountered the crashes with a release build and the following layout test invocation: run-webkit-tests --guard-malloc

Created attachment 362156 [details] Patch for landing

(In reply to Ryan Haddad from comment #93) > (In reply to Ross Kirsling from comment #91) > > What is GuardMalloc and how would I test with it on a Mojave machine? > It is a testing mode to find memory access bugs. I encountered the crashes > with a release build and the following layout test invocation: > run-webkit-tests --guard-malloc Thanks! Will be sure to make use of that in the follow-up.

Comment on attachment 362156 [details] Patch for landing Clearing flags on attachment: 362156 Committed r241620: <https://trac.webkit.org/changeset/241620>

All reviewed patches have been landed. Closing bug.

Comment on attachment 362156 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=362156&action=review > Source/WTF/wtf/Environment.h:39 > +inline const char* getRaw(const String& variable) > +{ > + auto string = get(variable); > + return string ? string->utf8().data() : nullptr; > +} This method is returning freed memory which is causing crashes on GuardMalloc and ASan bots. The const char* pointer being returned is owned by the WTF::String contained in the WTF::Optional<WTF::String>. Once `string` goes out of scope, the memory is freed. This function should probably return a std::unique_ptr<> to a const char* to manage its lifetime, or (better yet?) just inline getenv(variable.utf8().data()) for POSIX systems, which is what the code it's replacing did.

(In reply to WebKit Commit Bot from comment #96) > Comment on attachment 362156 [details] > Patch for landing > > Clearing flags on attachment: 362156 > > Committed r241620: <https://trac.webkit.org/changeset/241620> Reverted: Committed r241654: <https://trac.webkit.org/changeset/241654> "Causes use-after-free crashes running layout tests with ASan and GuardMalloc." (Requested by ddkilzer on #webkit.)

(In reply to David Kilzer (:ddkilzer) from comment #98) > string->utf8().data() Such a common mistake, and no clear way to make the CString interface safer. :/

(In reply to Michael Catanzaro from comment #100) > (In reply to David Kilzer (:ddkilzer) from comment #98) > > string->utf8().data() > > Such a common mistake, and no clear way to make the CString interface safer. > :/ Ugh, right... "necessary but slightly less convenient" was the intention with getRaw, but botching lifetimes certainly was not. I suppose we could just make getRaw a literal passthrough to std::getenv, as CurlContext's EnvironmentVariableReader (part of the inspiration for this patch) was doing: https://github.com/WebKit/webkit/blob/82e5788b7127fde8aa30bbf72c11b648cf5cfe67/Source/WebCore/platform/network/curl/CurlContext.cpp#L52 This won't handle multibyte text on Windows correctly, but if the function is understood to be use-at-your-own-risk...?

I’m not thrilled with this. I am not sure we should be adding this platform layer. Do we need our own interface to environment variables? Is this so heavily used throughout WebKit that this will truly be helpful? This same kind of issue came up before with international text. Someone was building a platform layer that we used instead of calling ICU directly, and eventually we chose not to add one because we didn’t want to layer a WebKit abstraction on top of everything from ICU. We need to be careful that we don’t create our own WebKit versions of things unnecessarily. I see some comments early on about what problems we might be solving, but I am not completely clear on what they are. I would like to understand explicitly what problems specifically we are trying to solve here. We should attempt to create the lightest possible layer to solve them. It's very easy to introduce problems. To pick one example, conversion of strings from 8-bit characters to and from WTF::String can easily cause problems if we don’t know what the character sets are (are they UTF-8, Latin-1, guaranteed to be ASCII, etc.). I don’t know that all environment variables are consistent in that respect. And it's easy to make something that's poor performance.

I would suggest that ideally WebKit should be able to work on platforms that don’t have environment variables at all. I don’t see why we’d want to depend on them. Maybe we can get rid of the getenv/setenv rather than making them easier to program with? Or at least put them inside platform-specific code rather than core WebKit code.

Although the original idea here is not my own, the benefits I see are the following: 1. A String-based interface -- in particular, functions like hasValue and getInt avoid the need for strcmp and sscanf in common operations. 2. ASSERTs to improve thread safety, coarse-grained as they may be -- we can't prevent getenv from being called all over the place, but we can actively seek to restrict where [un]setenv gets called. 3. Consistency improvements due to a platform-independent interface: - Do away with g_*env (the usage of which is currently inconsistent and debatably superfluous). - Use GetEnvironmentVariableW for proper multibyte charset support on Windows (which is disgustingly longwinded and deserves its own helper if we're not just going to cut corners with std::getenv). - Generally-speaking, avoid the worry over whether charsets are being handled correctly at every single getenv callsite (based on the hopefully-not-mistaken understanding that String::fromUTF8(getenv(...)) is appropriate for POSIX across the board). Other folks may have further points to make (or corrections to the above).

Created attachment 362399 [details] Patch

Attachment 362399 [details] did not pass style-queue: ERROR: Source/WTF/wtf/DataLog.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/posix/EnvironmentPOSIX.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/win/EnvironmentWin.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 84 files If any of these errors are false positives, please file a bug against check-webkit-style.

For now, here is a patch that addresses the UAF (by having getRaw be a literal std::getenv passthrough). We still need to decide whether there's sufficient justification for this API, but here's one more point I thought of for the list: 4. There are 71 unique env var literals touched in this patch, and that is certainly not the full list of env vars handled by WK -- minimizing them is surely a good idea but would be a massive undertaking.

TBH I was just thinking the other day that it would be nice to have a WebKit-level wrapper around ICU, simply because the ICU API is hard to use correctly and a recent bug could have been avoided with a sane wrapper. Weaning WebKit away from using environment variables sadly does not seem very practical, but fortunately I think almost all of them are mainly there just for debugging.

(In reply to Michael Catanzaro from comment #108) > TBH I was just thinking the other day that it would be nice to have a > WebKit-level wrapper around ICU, simply because the ICU API is hard to use > correctly and a recent bug could have been avoided with a sane wrapper. I’d like to hear more about that, but maybe this bug is not the place to discuss this. We exclusively use ICU’s C API, likely because of concerns about ABI stability; maybe the C++ one is easier to use? I’m concerned about WebKit having special solutions to every problem, rather than relying on standard solutions that are already widely available and understood. Our platform layers and utility libraries tend to grow and personally, I like to do what I can to shrink them.

Given the described benefits and the actual state of the patch itself, are there further conditions that would need to be met in order to be considered justifiable? Note that the per-platform implementations are in fact maximally thin (given a String-based interface) and that the helper functions simplify code in many locations throughout the codebase. The abstract concern over not using the POSIX API directly is the singular negative I'm seeing, but note that after this patch, the only bare calls that remain are in places that can't use the WTF API without non-trivial modification, so potentially this gives us a way to identify areas for improvement with respect to env var management (most notably, in the places where we'd be hitting the ASSERTs).

I agree with Darin's concerns. It's not clear to me that the layer is worth the benefit, it's not clear to me what the encoding guarantees are, and it would be nice to get rid of the use of environment variables anyway (having WebKit open to input via environment variables seems like a possible security issue). But that aside, I mostly wanted to ask about the use of 'const String&' in the interface. In all of the places I spot-checked, the environment variable names are being passed by the caller as a 'const char*' or a string literal. As the Environment interface stands, these variable names will be used to create String objects, which then internally are essentially converted back to a 'const char*'. There doesn't seem to be any benefit to having the interface take String references, while at the same time it eats up memory and CPU cycles. Perhaps a StringView would be more appropriate, if there's some reason for not just taking a 'const char*'?

(In reply to Keith Rollin from comment #111) > I agree with Darin's concerns. It's not clear to me that the layer is worth > the benefit, it's not clear to me what the encoding guarantees are, and it > would be nice to get rid of the use of environment variables anyway (having > WebKit open to input via environment variables seems like a possible > security issue). > > But that aside, I mostly wanted to ask about the use of 'const String&' in > the interface. In all of the places I spot-checked, the environment variable > names are being passed by the caller as a 'const char*' or a string literal. > As the Environment interface stands, these variable names will be used to > create String objects, which then internally are essentially converted back > to a 'const char*'. There doesn't seem to be any benefit to having the > interface take String references, while at the same time it eats up memory > and CPU cycles. Perhaps a StringView would be more appropriate, if there's > some reason for not just taking a 'const char*'? You're absolutely right, there's no excuse for it not at least being a StringView! The only reason it's not `const char*` is it because it seemed perhaps inappropriate to *require* a raw string (at least for the values if not for the variable names), although this isn't necessarily a strong argument since, as you mentioned, it's not a problem for any existing callsites. --- Of the points I've made, I can understand if (1) seems a mere nice-to-have and if, in spite of (4), we don't want to encourage even broader use of environment variables by the introduction of a new API. I would like to think that (3) is a reasonably strong argument, but if the non-Unix code in this patch doesn't actually modify any environment variables, then perhaps the worry over having a platform-independent [un]setenv is less compelling. One way or another, (2) deserves recognition for the problem that it is, whether or not we like the proposed assertion. I suppose you could argue that seeking out existing unsafe [un]setenv calls and preventing future ones is not in itself sufficient justification for a new API, but then it seems we should be discussing alternatives.

One other issue with the current situation is that it seems that getenv()'s Windows behavior is such that using it in a UNICODE application may be problematic and have performance implications due to having two environments to maintain. That may not be a huge issue if the only uses are for flags, but if any of the environment variable uses are for paths, then this is potentially a problem as it's unclear what happens to unicode paths in the environment that aren't representable in the current mbcs code page.

(In reply to Stephan Szabo from comment #113) > One other issue with the current situation is that it seems that getenv()'s > Windows behavior is such that using it in a UNICODE application may be > problematic and have performance implications due to having two environments > to maintain. > > That may not be a huge issue if the only uses are for flags, but if any of > the environment variable uses are for paths, then this is potentially a > problem as it's unclear what happens to unicode paths in the environment > that aren't representable in the current mbcs code page. Indeed -- so then at a minimum, we'll need the fixes for CURL_LOG_FILE, CURL_COOKIE_JAR_PATH, and CURL_CA_BUNDLE_PATH as well as the fixes for the three copies of LoggingWin.cpp, and these will all be repeating the same half-dozen lines. So then we certainly need a Windows-oriented "use me instead of getenv" replacement, but if this isn't a proper platform abstraction, then any cross-platform code is almost certain to forget to use it (or it'll just happen implicitly, e.g. via JSC's Options.cpp). So I think that bolsters argument (3) on the basis of getenv alone.

Something we have done for Windows before is to make Windows covers that rationalize Windows behavior and make it work like Linux in WebKit source files, using macros and the like. This was a really successful strategy in the past. Maybe we can do that for getenv, using a macro to map it to a version of getenv that does exactly what we want on Windows, something that we put in WTF. The callers can still just pretend they are calling the "real" getenv.

(In reply to Darin Adler from comment #115) > Something we have done for Windows before is to make Windows covers that > rationalize Windows behavior and make it work like Linux in WebKit source > files, using macros and the like. This was a really successful strategy in > the past. > > Maybe we can do that for getenv, using a macro to map it to a version of > getenv that does exactly what we want on Windows, something that we put in > WTF. > > The callers can still just pretend they are calling the "real" getenv. I think at this point we should step back a bit and consider what problem we're trying to solve with a macro-based solution like that, which is potentially quite confusing. We've also had trouble in the past caused by sneakily redefining standard things (e.g. our custom std::optional). Avoiding an unnecessary abstraction layer is a laudable goal. But if it's actually useful to have the abstractions -- and in this case, it sounds so to me, from the arguments above -- then that's fine.

OK I understand what you are saying, but I don’t agree.

It seems like we're all at an impasse over this patch. The original idea with this bug was that we weren't being consistent in our usage of environment variables. There were many cases of platform specific calls, g_get_env, GetEnvironmentVariableW. On top of that there were things like conversions to different types. There was some code done in cURL that had some of those conversions. Michael also mentioned the potential safety issues around threading and using environment variables. So this seemed like a neighborly thing to do from our end hence the patch. Darin has a great point that environment variables are a horrible way to configure things. I don't think anyone can disagree with that. However it'll be some amount of work to remove those environment variables throughout the codebase. So taking a step back the problem we want to solve is runtime configuration. With environment variables they're primarily used during development, though Michael mentioned that in some cases GTK users twiddle some to help debug problems in the wild. I'm wondering if its a better approach to have a unified concept of settings for running WebKit code. When it comes to environment variables that can be a backend for getting those values. That path could only be enabled for DEVELOPER_MODE so we can still use these things for testing and during development. There could be other paths for setting these values. If Apple plans on releasing iTunes as a Universal Windows Platform application environment variables are strictly disallowed so maybe theres a Windows Registry backend in that case. GLib has the concept of GSettings so maybe thats a GLib backend. Anyways that could be a longer term plan to get these things under control. At this point we can land this patch as is or just keep things as they are. The benefit of landing is additional safety and having a common interface across platforms. It also gives us a common API to grep for if we want to try for some unified settings in the future. I don't personally think this patch means that we approve of using environment variables for configuration though I can see that being an assumption considering we're exposing an API in this patch. If we think of this as a stepping stone to something better perhaps its more palatable. Anyways we'd like to know if this can land or if we just need to back off this patch. Thanks for all the constructive criticism on this.