1553850 - Consider enabling sandboxing on pgo profiling runs

Status: NEW

(Firefox Build System :: General, enhancement, P3)

Description

[:Gijs (he/him)]

In bug 1196094 I'm adding support for e10s being enabled during profiling runs. Unfortunately, sandboxing interferes with this (at least locally, the profiling run hangs even just on the run that only opens the data: URI with a quitter call), presumably because the instrumentation code is unhappy when writing to the profile output files fails. I don't really know what the best solution for this is, the simplest seemed to just disable sandboxing while profiling. It's possible this is in fact also the best solution (that is, adding profiling-only product code seems counterproductive in terms of profiling being as close to "real world" as possible), I'm not entirely sure, but I wanted to get this on file so people who know about this can offer opinions/advice...

Comment 1

[Nathan Froyd [:froydnj]]

Adding cc for people on the relevant platforms. I assume this is the sandboxing code prohibiting access on the profile file writes, but maybe there's something more insidious here?

Comment 2

[Bob Owen (:bobowen)]

Where does it try to write the profiling data and how much control do we have over it?

Comment 3

[Nathan Froyd [:froydnj]]

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Ideally, there'd be a way to get the profiling runtime to open the files in early startup (which may not be a complete answer on Windows, because the process can be started with different permissions from normal, but I don't know all the details there).

Failing that, we'd have to make PGO-specific changes to the sandbox policies. Naively I'd think that that would be better than disabling sandboxing, because we'd still get profiling information on the sandbox code itself — on Linux and Windows we transparently intercept and broker some operations with code that's in our tree and could possibly benefit from PGO — and the changes should be relatively small.

Comment 5

[Bob Owen (:bobowen)]

(In reply to Nathan Froyd [:froydnj] from comment #3)

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

We could add the path specified in the env var to the rules on Windows and other platforms I think.
We do a similar thing for MOZ_LOG on Windows.
(Ideally, eventually, we'd like MOZ_LOG and other logging to pass through FDs/HANDLEs to the process, where we have control of things, but I guess for this we wouldn't be able to do that.)

Comment 6

[Michael Shal [:mshal]]

(In reply to Bob Owen (:bobowen) from comment #5)

(In reply to Nathan Froyd [:froydnj] from comment #3)

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

We could add the path specified in the env var to the rules on Windows and other platforms I think.
We do a similar thing for MOZ_LOG on Windows.
(Ideally, eventually, we'd like MOZ_LOG and other logging to pass through FDs/HANDLEs to the process, where we have control of things, but I guess for this we wouldn't be able to do that.)

Is that something that can be wrapped in an ifdef MOZ_PROFILE_GENERATE? I think changing when the files get opened would be difficult, and would require us to patch llvm.

Comment 7

[Bob Owen (:bobowen)]

(In reply to Michael Shal [:mshal] from comment #6)

(In reply to Bob Owen (:bobowen) from comment #5)

(In reply to Nathan Froyd [:froydnj] from comment #3)

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

We could add the path specified in the env var to the rules on Windows and other platforms I think.
We do a similar thing for MOZ_LOG on Windows.
(Ideally, eventually, we'd like MOZ_LOG and other logging to pass through FDs/HANDLEs to the process, where we have control of things, but I guess for this we wouldn't be able to do that.)

Is that something that can be wrapped in an ifdef MOZ_PROFILE_GENERATE? I think changing when the files get opened would be difficult, and would require us to patch llvm.

If this has its own build, I think that opens up more options.
We could either use LLVM_PROFILE_FILE (if this already exists) or if we know where the files are created we could just open up that folder (this could be controlled by our own env var if we like, in case it needs to changed).
Either way we can put this all in an ifdef MOZ_PROFILE_GENERATE, so the normal build isn't affected.

Comment 8

[Haik Aftandilian [:haik]]

What Bob wrote in comment 7 applies to Mac as well. If profiling runs have MOZ_PROFILE_GENERATE defined, it should be starightforward to whitelist LLVM_PROFILE_FILE in the Mac sandbox rules without affecting normal builds.

Comment 9

[Haik Aftandilian [:haik]]

Adding needinfo for myself to test Mac PGO build locally and debug sandboxing issue.

Comment 10

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #7)

If this has its own build, I think that opens up more options.
We could either use LLVM_PROFILE_FILE (if this already exists) or if we know where the files are created we could just open up that folder (this could be controlled by our own env var if we like, in case it needs to changed).
Either way we can put this all in an ifdef MOZ_PROFILE_GENERATE, so the normal build isn't affected.

LLVM_PROFILE_FILE has substitution bits in it that get replaced by bits we don't control - can we use wildcards in the things we allow through the sandbox? And could you potentially link me to where we'd make such a change (to apply to all our sandboxed child processes) ? I'd like to work on this considering what we ran into in bug 1663424.

Comment 11

[(Away)]

IIUC, LLVM_PROFILE_FILE doesn't have to be strictly a filename, it could contain a path to a directory that we use just for profiles, and then open up that directory.

Embarrassing edit: we already do this. :-) https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/testing/mozharness/scripts/android_emulator_pgo.py#199-207

Comment 12

[:Gijs (he/him)]

(In reply to :dmajor from comment #11)

IIUC, LLVM_PROFILE_FILE doesn't have to be strictly a filename, it could contain a path to a directory that we use just for profiles, and then open up that directory.

Embarrassing edit: we already do this. :-) https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/testing/mozharness/scripts/android_emulator_pgo.py#199-207

Ah, hm, we do on Android but not on desktop - https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/build/pgo/profileserver.py#132 . But that makes sense, I guess we can make it some directory, update the code collecting them, and safelist the directory for sandbox access. Still, not sure what the best place is to do that... :-)

Comment 13

[(Away)]

Ah, right, I was getting confused with the other bug from today, and thought you were only talking about Android.

Comment 14

[Bob Owen (:bobowen)]

(In reply to :Gijs (he/him) from comment #10)

(In reply to Bob Owen (:bobowen) from comment #7)

If this has its own build, I think that opens up more options.
We could either use LLVM_PROFILE_FILE (if this already exists) or if we know where the files are created we could just open up that folder (this could be controlled by our own env var if we like, in case it needs to changed).
Either way we can put this all in an ifdef MOZ_PROFILE_GENERATE, so the normal build isn't affected.

LLVM_PROFILE_FILE has substitution bits in it that get replaced by bits we don't control - can we use wildcards in the things we allow through the sandbox? And could you potentially link me to where we'd make such a change (to apply to all our sandboxed child processes) ? I'd like to work on this considering what we ran into in bug 1663424.

We certainly do on Windows pattern rules are here [1], I seem to remember it can't normally start with a wildcard either.
If we wanted to add the same rule for all sandboxed child processes then in SandboxBroker::LaunchApp [2] is probably best.
We already have rules for MOZ_LOGging and DEBUG access to %TEMP% for refcount logging IIRC.

I think we do for Linux and Mac, but jld and haik would be the best people to give details.

[1] https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/security/sandbox/chromium/sandbox/win/src/sandbox_policy.h#229
[2] https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#241

Comment 15

[Haik Aftandilian [:haik]]

For Mac, we could change StartMacSandbox() to pass a directory or file path to each sandbox profile that needs this. This would be similar to how the HOME environment variable is passed[1] to sandbox profiles[2] now via the HOME_PATH param. Then in the policy we could add a new rule to allow read or write access to the path provided in the new param. Regexes are possible. Since this has its own build, the only security consideration would be to make sure the new rules could only be used for MOZ_PROFILE_GENERATE builds.

1. https://searchfox.org/mozilla-central/rev/8a0745cd346f0cfb89ae71690babbf7bff706113/security/sandbox/mac/Sandbox.mm#297
2. https://searchfox.org/mozilla-central/rev/8a0745cd346f0cfb89ae71690babbf7bff706113/security/sandbox/mac/SandboxPolicyContent.h#25

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

For Linux, this is in SandboxBrokerPolicyFactor.cpp and it sounds like this will need something with the AddPrefix method, like what we do for XPCOM_MEM_BLOAT_LOG.