Bug 194612 – EnvironmentUtilities::stripValuesEndingWithString isn't thread-safe

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Ross Kirsling]

EnvironmentUtilities::stripValuesEndingWithString isn't thread-safe -- its callsites in Source/ might(?) be, but its API test isn't (nor can it be, since WTF::initializeThreading() is called before the test even begins).

Since this function does little other than string manipulation, we could replace it with a function that *only* does string manipulation and let getenv/setenv be called separately.

[Alexey Proskuryakov]

> nor can it be, since WTF::initializeThreading() is called before the test even begins

Can you clarify why this means that the test will have trouble with threading?

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #1)
> > nor can it be, since WTF::initializeThreading() is called before the test even begins
> 
> Can you clarify why this means that the test will have trouble with
> threading?

Admittedly, even though it can't be called "thread-safe", the test would be unlikely to ever encounter a problem, since the env vars it's setting are not realistic.

The issue is just that it would be nice to use the new WTF::Environment::set (from bug 192405), but this currently ASSERTs since "has threading been initialized yet?" is really our only way of ensuring thread safety of setenv (coarse-grained as that approach may be).

[Alexey Proskuryakov]

Are you saying that you see the fact that initializeThreading was called as a sign that any code could be running on a secondary thread? I don't think that is closely enough related to be relevant here.

There are always many threads running in any process, whether it calls initializeThreading or not. It's never safe for other threads to manipulate the environment, except for when the code is fully in control of the process (so maybe the dynamic linker could safely do something here).

Can you describe the specific scenario that you are concerned about? "Thread safe" has many different meanings, so it's rarely a good way to describe threading related requirements or issues.

[Ross Kirsling]

This approach arose in https://bugs.webkit.org/show_bug.cgi?id=192405#c11 as a way to try to avoid some of the hard-to-repro bugs prevalent in GLib and such described here: https://blogs.gnome.org/mcatanzaro/2018/02/28/on-setenv-and-explosions/

Sorry if my word choice has added any confusion -- the basic concept is that we can't hope to prevent getenv from being littered anywhere and everywhere, so the best we can do is be aggressive in restricting where [un]setenv is used.

[Michael Catanzaro]

(In reply to Alexey Proskuryakov from comment #3)
> Are you saying that you see the fact that initializeThreading was called as
> a sign that any code could be running on a secondary thread? I don't think
> that is closely enough related to be relevant here.

Right: it's kinda a big hammer, and not even accurate enough since it's of course possible to have non-WTF threads before calling WTF::initializeThreading. (Maybe there are pthread APIs to detect if threads have been created; not sure.) And these APIs should never be used in the UI process under any circumstances. But anyway, this check will catch the vast majority of potential misuse, so I think it's a good check to have.

> Can you describe the specific scenario that you are concerned about? "Thread
> safe" has many different meanings, so it's rarely a good way to describe
> threading related requirements or issues.

Goal is to prevent use of Environment::set and Environment::unset except *very* early in WebKit's initialization. (Ideally also only in secondary processes, although it's not clear how we could enforce that.) Using setenv() or unsetenv() directly would bypass these checks, but there's not much we can do about that besides add a style checker rule.

There's more discussion in https://bugs.webkit.org/show_bug.cgi?id=194370#c6 (a different bug from the one mentioned above).

[Ross Kirsling]

Making a note here that whatever we do here needs to be verified with `run-webkit-tests --guard-malloc` on Mac.

[Ross Kirsling]

Created attachment 362539 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 362539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362539&action=review

> Source/WebKit/ChangeLog:8
> +        We shouldn't be writing API tests that get, modify, and check environment variables.

Is this true? I don't see what the problem with tests that do that is.

[Ross Kirsling]

While I'm waiting for permission to reland bug 192405, here's a solution for this bug that just uses the POSIX API.

This patch replaces stripValuesEndingWithString with two very simple functions which just do string-processing and env var-processing -- the former is appropriate for API testing, the latter is still justified since the functionality is needed in three places.

I did copy over WTF::threadingIsInitialized() from bug 192405 so that we can ASSERT that it's false.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 362539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362539&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        We shouldn't be writing API tests that get, modify, and check environment variables.
> 
> Is this true? I don't see what the problem with tests that do that is.

That is at least the proposition I'm basing this on. If we think it's an overstatement then I can replace the text somehow, but unless we have a better way to try and ensure setenv safety, it is a fact that it's already "too late" to use setenv by the time an API test has begun. :(

[Alexey Proskuryakov]

Comment on attachment 362539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362539&action=review

>>> Source/WebKit/ChangeLog:8
>>> +        We shouldn't be writing API tests that get, modify, and check environment variables.
>> 
>> Is this true? I don't see what the problem with tests that do that is.
> 
> That is at least the proposition I'm basing this on. If we think it's an overstatement then I can replace the text somehow, but unless we have a better way to try and ensure setenv safety, it is a fact that it's already "too late" to use setenv by the time an API test has begun. :(

It seems fine and legitimate to change the environment in tests, so adding functionality that explicitly forbids that looks wrong to me.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #11)
> It seems fine and legitimate to change the environment in tests, so adding
> functionality that explicitly forbids that looks wrong to me.

I think it's noteworthy that no other API tests do so though (GLib platforms set some from test server main() functions but that's the type of usage one would be inclined to expect). And even if we are okay with modifying the environment in tests, I hope it's fair to say that we shouldn't be testing whether we successfully modified the actual environment...right?

[Ross Kirsling]

Created attachment 362557 [details]
Patch

[Ross Kirsling]

WinCairo failure here is a fluke...

[Ross Kirsling]

Anyway, if the ASSERT is an unresolved discussion from bug 192405, then it doesn't actually need to be part of this patch; it's really just a way to demonstrate the issue. The patch should stand on its own merits without it -- after all, we have TestWTF.WTF.StringOperators as a cautionary tale about what can happen when "unit" tests aren't properly self-contained!

[Alexey Proskuryakov]

Splitting the function in two parts seems reasonable to me, but I'm still unimpressed by threadingIsInitialized and by the ASSERT.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #16)
> Splitting the function in two parts seems reasonable to me, but I'm still
> unimpressed by threadingIsInitialized and by the ASSERT.

Fair enough. The conversation on the other bug is still quite open, so I'll just eliminate that part here.

[Ross Kirsling]

Created attachment 362659 [details]
Patch

[Alexey Proskuryakov]

I won't have a chance to review line by line, but the direction looks good to me. Thank you!

What is the difference between EXPECT_STREQ and EXPECT_EQ in practical terms? Please explain the change in ChangeLog.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #19)
> What is the difference between EXPECT_STREQ and EXPECT_EQ in practical
> terms? Please explain the change in ChangeLog.

It's just strcmp vs. operator==, since the return type changed. I can add that to the ChangeLog though, sure.

[Ross Kirsling]

Alex or Chris, could you take a look at this? It's really simple, it just happens to require an owner.

[Ross Kirsling]

Anyone have a moment to review here?

[Alex Christensen]

Comment on attachment 362659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362659&action=review

Seems good to me.  This simplifies the code quite a bit.  I guess the motivation was just to make the API test more reliable, right?

> Source/WebKit/Platform/unix/EnvironmentUtilities.cpp:41
> +    auto hasAppended = false;

StringBuilder::isEmpty could be used instead of this additional boolean tracking the same thing.

[Ross Kirsling]

(In reply to Alex Christensen from comment #23)
> Comment on attachment 362659 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362659&action=review
> 
> Seems good to me.  This simplifies the code quite a bit.  I guess the
> motivation was just to make the API test more reliable, right?

Yessir.


> > Source/WebKit/Platform/unix/EnvironmentUtilities.cpp:41
> > +    auto hasAppended = false;
> 
> StringBuilder::isEmpty could be used instead of this additional boolean
> tracking the same thing.

Unfortunately, if the first preserved entry is empty, then output.isEmpty() is still false at the first point where we need to insert a separator ':'.
I figured people might have that exact knee-jerk reaction, but it seems avoidable...

[Ross Kirsling]

(In reply to Ross Kirsling from comment #24)
> ... then output.isEmpty() is still false ...

Whoops, still *true*, I mean.

[Ross Kirsling]

Created attachment 363377 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 363377 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 363377 [details]
Patch for landing

Clearing flags on attachment: 363377

Committed r242292: <https://trac.webkit.org/changeset/242292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48525786>

[Darin Adler]

Switching from STREQ to plain EQ is not so great for tests. I’d prefer to use other techniques so we can continue to use STREQ and get good output when the strings don't match. There are examples in various other tests using things like utf8().data() for compatibility with STREQ.

[Ross Kirsling]

(In reply to Darin Adler from comment #31)
> Switching from STREQ to plain EQ is not so great for tests. I’d prefer to
> use other techniques so we can continue to use STREQ and get good output
> when the strings don't match. There are examples in various other tests
> using things like utf8().data() for compatibility with STREQ.

Makes sense. I just did this since it's a lifetime violation to return utf8().data(), but I can place it at each callsite if STREQ is worth preserving.

[Ross Kirsling]

Committed r242323: <https://trac.webkit.org/changeset/242323>

[Darin Adler]

STREQ is worth preserving because you can see what's wrong when there’s a failure, whereas with EQ you end up seeing addresses, not strings, in the output. On success the two are the same.

However, I think it’s reasonable to continue to look for a more elegant way to do it, since explicit utf8().data() on every line are ugly. Various tests use different tricks to avoid that. Would be nice if there was a clean standard way to do it for new tests.

[Ross Kirsling]

(In reply to Darin Adler from comment #34)
> However, I think it’s reasonable to continue to look for a more elegant way
> to do it, since explicit utf8().data() on every line are ugly. Various tests
> use different tricks to avoid that. Would be nice if there was a clean
> standard way to do it for new tests.

I'm seeing utf8().data() at callsites much more than code explicitly written to avoid it, but I guess AtomicString does a strncpy into a static char buffer (which seems...worse) while ApplicationManifestParser brings EXPECT_STREQ *into* the helper function (which is a lovely idea that I hadn't considered!):
https://github.com/WebKit/webkit/blob/master/Tools/TestWebKitAPI/Tests/WTF/AtomicString.cpp#L64-L69
https://github.com/WebKit/webkit/blob/master/Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp#L82-L87

I'd be happy to adopt the latter approach here.

[Ross Kirsling]

Committed r242338: <https://trac.webkit.org/changeset/242338>