Consider enabling sandboxing on pgo profiling runs
Categories
(Firefox Build System :: General, enhancement, P3)
Tracking
(firefox137 fixed)
Tracking | Status | |
---|---|---|
firefox137 | --- | fixed |
People
(Reporter: Gijs, Assigned: gerard-majax)
References
Details
(Keywords: in-triage)
Attachments
(6 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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•6 years ago
|
||
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•6 years ago
|
||
Where does it try to write the profiling data and how much control do we have over it?
![]() |
||
Comment 3•6 years ago
|
||
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•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 5•6 years ago
|
||
(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.)
Updated•6 years ago
|
Comment 6•6 years ago
|
||
(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•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
Adding needinfo for myself to test Mac PGO build locally and debug sandboxing issue.
Reporter | ||
Comment 10•5 years ago
|
||
(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•5 years ago
•
|
||
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
Reporter | ||
Comment 12•5 years ago
|
||
(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•5 years ago
|
||
Ah, right, I was getting confused with the other bug from today, and thought you were only talking about Android.
Comment 14•5 years ago
|
||
(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
Updated•5 years ago
|
Comment 15•5 years ago
|
||
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.
Comment 16•5 years ago
|
||
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
.
Updated•3 years ago
|
Assignee | ||
Comment 17•11 months ago
|
||
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
Assignee | ||
Comment 18•11 months ago
|
||
Assignee | ||
Comment 19•11 months ago
|
||
(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
Comment 20•11 months ago
|
||
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.
Assignee | ||
Updated•11 months ago
|
Assignee | ||
Comment 21•11 months ago
|
||
Assignee | ||
Comment 22•11 months ago
|
||
Assignee | ||
Comment 23•11 months ago
|
||
Updated•11 months ago
|
Updated•11 months ago
|
Assignee | ||
Comment 24•11 months ago
|
||
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Assignee | ||
Comment 25•11 months ago
|
||
Comment 26•10 months ago
|
||
Comment 27•10 months ago
|
||
bugherder |
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•10 months ago
|
||
Comment 29•10 months ago
|
||
Backed out as requested by Gerard for causing snap canonical builds bustages.
Comment 30•10 months ago
|
||
Assignee | ||
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Assignee | ||
Comment 31•10 months ago
|
||
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
Assignee | ||
Comment 32•10 months ago
|
||
(In reply to :gerard-majax from comment #31)
I am not sure yet why, but under some circumstances, I get the
Allow()
ofopenat
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
Assignee | ||
Comment 33•5 months ago
|
||
Updated•5 months ago
|
Comment 34•5 months ago
|
||
Comment 35•5 months ago
|
||
bugherder |
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•5 months ago
|
||
It caused gtest failures.
[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
Assignee | ||
Updated•5 months ago
|
Comment 37•5 months ago
|
||
Backout merged into central.
Comment 38•4 months ago
|
||
Reopened as per comment #37.
Updated•4 months ago
|
Comment 39•4 months ago
|
||
Comment 40•4 months ago
|
||
bugherder |
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
Description
•