WebKit2 crashes when initializing due to not-threadsafe call to _NSGetEnviron()

• Component: WebKit2
• Status: RESOLVED INVALID

[white]

When WebKit2 is to initialize, it calls _NSGetEnviron() to get all env variables.
The code is as below: ( in file: Options.cpp, method: void Options::initialize() )

#if PLATFORM(COCOA)
            bool hasBadOptions = false;
            for (char** envp = *_NSGetEnviron(); *envp; envp++) {
                const char* env = *envp;
                if (!strncmp("JSC_", env, 4)) {
                    if (!Options::setOption(&env[4])) {
                        dataLog("ERROR: invalid option: ", *envp, "\n");
                        hasBadOptions = true;
                    }
                }
            }

However, _NSGetEnviron is not thread-safe. If there is another thread calling putenv(),
there is a small chance that WebKit crashes due to invalid pointer sent to strncpm().
This is due to putenv() is using 'realloc' when necessary, which invalidates the old pointer values.

[Kimmo Kinnunen]

I think modifying environment is inherently single-threaded operation.
Is there a place in WebKit which modifies the environment in such a way that causes WebKit to crash in this location during initialisation?

[white]

(In reply to Kimmo Kinnunen from comment #1)
> I think modifying environment is inherently single-threaded operation.
> Is there a place in WebKit which modifies the environment in such a way that
> causes WebKit to crash in this location during initialisation?

AFAIK there isn't. 

This crashing is due to some third party code naively called putenv() in non-main threads and caused racing condition issues while WebKit2 was initializing itself in main thread.

[Alexey Proskuryakov]

Given the above, is there a WebKit issue to track here?

[Kimmo Kinnunen]

(In reply to white from comment #2)
> (In reply to Kimmo Kinnunen from comment #1)
> > I think modifying environment is inherently single-threaded operation.
> > Is there a place in WebKit which modifies the environment in such a way that
> > causes WebKit to crash in this location during initialisation?
> 
> AFAIK there isn't. 
> 
> This crashing is due to some third party code naively called putenv() in
> non-main threads and caused racing condition issues while WebKit2 was
> initializing itself in main thread.

So spelled out what Alexey mentioned:
There are two options:
A) Third-party code modifies the environment in single-threaded manner
or
B) WebKit should never use the environment

I believe this option is impossible to get correct:
C) Third-party code modifies the environment in random threads at random points in time, WebKit uses the environment


I think B would be a progression in many ways, one of which would be the bug you mention, but this is just an opinion. However, there is a lot of environment use in WebKit, so it might not be such an easy task for what it accomplishes. Also, other components could use the environment.

This particular crash is a bit odd since intuitively the client process should not use JSC, and hence should not crash here. However, there is still the issue of other locations using the environment.

[Radar WebKit Bug Importer]

<rdar://problem/83155440>

[Alexey Proskuryakov]

> Also, other components could use the environment.

Other system frameworks certainly do use the environment. There is also a huge number of getenv calls in WebKit, not just this one _NSGetEnviron use in JavaScriptCore.