1553850 - Consider enabling sandboxing on pgo profiling runs

Status: RESOLVED FIXED

(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.

Comment 17

[:gerard-majax]

As a by product of bug 1908630 I have been able to get something together that should allow at least on Linux running with the sandbox enabled

Comment 18

[:gerard-majax]

Attachment: Bug 1553850 - Add LLVM profiling to Linux Sandbox r=gcp,afinder

Comment 19

[:gerard-majax]

(In reply to :gerard-majax from comment #17)

As a by product of bug 1908630 I have been able to get something together that should allow at least on Linux running with the sandbox enabled

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=a2eaf732ab81c606c502dd26d898dc4fd5c29c57&newProject=try&newRevision=4d41a104b0e794326faa78ff51710d098ad5bbb5

Comment 20

[Phabricator Automation]

Comment on attachment 9413897 [details]
Bug 1553850 - Add LLVM profiling to Linux Sandbox r=gcp,afinder

Revision D217167 was moved to bug 1909421. Setting attachment 9413897 [details] to obsolete.

Comment 21

[:gerard-majax]

Attachment: Bug 1553850 - Enable use of sandbox during profiling step r=afinder,glandium

Comment 22

[:gerard-majax]

Attachment: Bug 1553850 - Add LLVM_PROFILE_FILE directory computation r=bobowen,haik

Comment 23

[:gerard-majax]

Attachment: Bug 1553850 - Add LLVM profiling to macOS Sandbox r=haik

Comment 24

[:gerard-majax]

Attachment: Bug 1553850 - Add LLVM profiling to Windows Sandbox r=bobowen

Comment 25

[:gerard-majax]

https://beta--mozilla-perfcompare.netlify.app/compare-results?baseRev=91bd402c4a6b356e17692bc9097617694dfa836e&baseRepo=try&newRev=cced393e1be3d75ce6a1b5b05ed13ded2393a105&newRepo=try&framework=13

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ab954ad623a
Enable use of sandbox during profiling step r=perftest-reviewers,afinder,glandium
https://hg.mozilla.org/integration/autoland/rev/678defcc87e3
Add LLVM_PROFILE_FILE directory computation r=bobowen,haik
https://hg.mozilla.org/integration/autoland/rev/516100dff406
Add LLVM profiling to macOS Sandbox r=haik
https://hg.mozilla.org/integration/autoland/rev/ab269021ad5b
Add LLVM profiling to Linux Sandbox r=gcp,perftest-reviewers,afinder
https://hg.mozilla.org/integration/autoland/rev/ebe866c9c2b1
Add LLVM profiling to Windows Sandbox r=bobowen

Comment 27

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/7ab954ad623a
https://hg.mozilla.org/mozilla-central/rev/678defcc87e3
https://hg.mozilla.org/mozilla-central/rev/516100dff406
https://hg.mozilla.org/mozilla-central/rev/ab269021ad5b
https://hg.mozilla.org/mozilla-central/rev/ebe866c9c2b1

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ceadc1e3298
Backed out 4 changesets as requested by Gerard for causing snap canonical builds bustages. CLOSED TREE

Comment 29

[tszentpeteri]

Backed out as requested by Gerard for causing snap canonical builds bustages.

• Backout link
• Second Backout link

Comment 30

[tszentpeteri]

Backouts merged in central:
https://hg.mozilla.org/mozilla-central/rev/1ceadc1e3298b37dc256f12338f4bbde4ca4f1a8
https://hg.mozilla.org/mozilla-central/rev/96843dab7e74672df5d972a4fcb124c67627cb91

Comment 31

[:gerard-majax]

I am not sure yet why, but under some circumstances, I get the Allow() of openat syscall to somehow trigger a crash ; it seems to mess with gfx code as per bug 1914446, but it did not happens on CI, nor on my laptop ; it does happen on my desktop and archlinux/snap builds

Comment 32

[:gerard-majax]

(In reply to :gerard-majax from comment #31)

I am not sure yet why, but under some circumstances, I get the Allow() of openat syscall to somehow trigger a crash ; it seems to mess with gfx code as per bug 1914446, but it did not happens on CI, nor on my laptop ; it does happen on my desktop and archlinux/snap builds

somehow i dont understand how it could have worked at all before, but we are hitting the fact we are chroot'd, so open() on the fonts fails because those dont exists. We'll have to go through the broker, which was tricky because of llvm profile data writes after the broken was dead at some point

Comment 33

[:gerard-majax]

Attachment: Bug 1553850 - Make the SandboxBroker live until the process exits.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac18e6bb1835
Enable use of sandbox during profiling step r=perftest-reviewers,afinder,glandium
https://hg.mozilla.org/integration/autoland/rev/f7f8df99ad1f
Add LLVM_PROFILE_FILE directory computation r=bobowen,haik
https://hg.mozilla.org/integration/autoland/rev/99ff1f091b19
Make the SandboxBroker live until the process exits. r=necko-reviewers,kershaw,jld
https://hg.mozilla.org/integration/autoland/rev/a5f077fc1e6a
Add LLVM profiling to macOS Sandbox r=haik
https://hg.mozilla.org/integration/autoland/rev/2dd45495e717
Add LLVM profiling to Windows Sandbox r=bobowen
https://hg.mozilla.org/integration/autoland/rev/281b3750a7f0
Add LLVM profiling to Linux Sandbox r=perftest-reviewers,afinder,jld

Comment 35

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ac18e6bb1835
https://hg.mozilla.org/mozilla-central/rev/f7f8df99ad1f
https://hg.mozilla.org/mozilla-central/rev/99ff1f091b19
https://hg.mozilla.org/mozilla-central/rev/a5f077fc1e6a
https://hg.mozilla.org/mozilla-central/rev/2dd45495e717
https://hg.mozilla.org/mozilla-central/rev/281b3750a7f0

Comment 36

[agoloman]

It caused gtest failures.

• Backout link
• Push with failures
• Failure Log

[task 2025-02-06T05:15:36.019Z] 05:15:36     INFO -  TEST-START | SandboxBrokerMisc.LeakCheck
[task 2025-02-06T05:15:36.050Z] 05:15:36     INFO -  [914] Sandbox: SandboxBroker: thread creation failed: ENOMEM
[task 2025-02-06T05:15:36.050Z] 05:15:36  WARNING -  TEST-UNEXPECTED-FAIL | SandboxBrokerMisc.LeakCheck | Value of: broker != nullptr
[task 2025-02-06T05:15:36.051Z] 05:15:36     INFO -    Actual: false
[task 2025-02-06T05:15:36.052Z] 05:15:36     INFO -  Expected: true
[task 2025-02-06T05:15:36.052Z] 05:15:36     INFO -   @ /builds/worker/checkouts/gecko/security/sandbox/linux/gtest/TestBroker.cpp:681
[task 2025-02-06T05:15:36.053Z] 05:15:36  WARNING -  TEST-UNEXPECTED-FAIL | SandboxBrokerMisc.LeakCheck | test completed (time: 31ms)
[task 2025-02-06T05:15:36.053Z] 05:15:36     INFO -  TEST-START | SandboxBrokerPolicyLookup.Simple

Comment 37

[agoloman]

Backout merged into central.

Comment 38

[Takanori MATSUURA]

Reopened as per comment #37.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/982a5ff9aad6
Enable use of sandbox during profiling step r=perftest-reviewers,afinder,glandium
https://hg.mozilla.org/integration/autoland/rev/96b6558dff15
Add LLVM_PROFILE_FILE directory computation r=bobowen,haik
https://hg.mozilla.org/integration/autoland/rev/68cd156cda7c
Make the SandboxBroker live until the process exits. r=necko-reviewers,kershaw,jld
https://hg.mozilla.org/integration/autoland/rev/06e90b560e09
Add LLVM profiling to macOS Sandbox r=haik
https://hg.mozilla.org/integration/autoland/rev/4847bd33e612
Add LLVM profiling to Windows Sandbox r=bobowen
https://hg.mozilla.org/integration/autoland/rev/7d44ca7b4af4
Add LLVM profiling to Linux Sandbox r=perftest-reviewers,afinder,jld
https://hg.mozilla.org/integration/autoland/rev/595c92211401
apply code formatting via Lando

Comment 40

[amarc]

https://hg.mozilla.org/mozilla-central/rev/982a5ff9aad6
https://hg.mozilla.org/mozilla-central/rev/96b6558dff15
https://hg.mozilla.org/mozilla-central/rev/68cd156cda7c
https://hg.mozilla.org/mozilla-central/rev/06e90b560e09
https://hg.mozilla.org/mozilla-central/rev/4847bd33e612
https://hg.mozilla.org/mozilla-central/rev/7d44ca7b4af4
https://hg.mozilla.org/mozilla-central/rev/595c92211401