1748366 - PR_SetEnv/setenv/putenv races with non-nspr reads from the environment

Status: NEW

(Core :: XPCOM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See bug 1748361 in NSPR. I believe the only fix for this issue is to eliminate all the PR_SetEnv/putenv/setenv calls within Firefox that might execute concurrently with getaddrinfo.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

From https://gist.github.com/comex/9fa0899293d1c6e748faa81b875b25ac, here are a list of environment variables that are known to be modified while there are multiple threads running. I'm not sure which of these might be running concurrently with Necko's address resolution. This is not a comprehensive list:

testbed firefox-esr[5545]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[5545]: sthack: putenv("XRE_BINARY_PATH=")
testbed firefox-esr[5545]: sthack: putenv("XRE_PROFILE_LOCAL_PATH=")
testbed firefox-esr[5545]: sthack: putenv("XRE_PROFILE_PATH=")
testbed firefox-esr[5545]: sthack: putenv("XRE_RESTARTED_BY_PROFILE_MANAGER=")
testbed firefox-esr[5545]: sthack: putenv("XRE_START_OFFLINE=")
testbed firefox-esr[5545]: sthack: putenv("XUL_APP_FILE=")
testbed firefox-esr[5747]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[5774]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[5837]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[6042]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")

Comment 2

[Daniel Veditz [:dveditz]]

Dragana: needinfo just to make sure you've seen this.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)

From https://gist.github.com/comex/9fa0899293d1c6e748faa81b875b25ac, here are a list of environment variables that are known to be modified while there are multiple threads running. I'm not sure which of these might be running concurrently with Necko's address resolution. This is not a comprehensive list:

testbed firefox-esr[5545]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[5545]: sthack: putenv("XRE_BINARY_PATH=")
testbed firefox-esr[5545]: sthack: putenv("XRE_PROFILE_LOCAL_PATH=")
testbed firefox-esr[5545]: sthack: putenv("XRE_PROFILE_PATH=")
testbed firefox-esr[5545]: sthack: putenv("XRE_RESTARTED_BY_PROFILE_MANAGER=")
testbed firefox-esr[5545]: sthack: putenv("XRE_START_OFFLINE=")
testbed firefox-esr[5545]: sthack: putenv("XUL_APP_FILE=")
testbed firefox-esr[5747]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[5774]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[5837]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")
testbed firefox-esr[6042]: sthack: putenv("__GL_ALLOW_FXAA_USAGE=0")

There aren't any necko threads running when these are done. I think the only threads running by that point are:

• one that executes lsb-release.
• one that reads ahead some libs.

(the latter is windows-only)

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

There aren't any necko threads running when these are done. I think the only threads running by that point are:
one that executes lsb-release.
one that reads ahead some libs.

According to the glibc docs: "Modifications of environment variables are not allowed in multi-threaded programs" without qualifications; see https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html.

However, subsequently to filing this issue I learned that besides getaddrinfo, there are other (libc) functions that read from the environment. So, Necko probably isn't the right component for this, as it's a more general thing.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #4)

There aren't any necko threads running when these are done. I think the only threads running by that point are:
one that executes lsb-release.
one that reads ahead some libs.

According to the glibc docs: "Modifications of environment variables are not allowed in multi-threaded programs" without qualifications; see https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html.

However, subsequently to filing this issue I learned that besides getaddrinfo, there are other (libc) functions that read from the environment. So, Necko probably isn't the right component for this, as it's a more general thing.

I agree with this. It looks like necko is not the right component for this.
Not sure if XPCOM is the right component. Feel free to change if you think it's not.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Not sure there's much we can do here. Fortunately to my knowledge the majority of places we set the environment happen fairly early during startup, so shouldn't impact normal operation.

Comment 7

[Gabriele Svelto [:gsvelto]]

FYI now that bug 1752703 landed all environment manipulation functions have to go through a single mutex, so this should be solved.