1583368 - Ensure we don't use sccache on dependencies of shippable builds

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Comment 1

[Michael Shal [:mshal]]

This should be as simple as adding the win64-sccache toolchain to the win32-shippable and win64-shippable builds in taskcluster/ci/instrumented-build/kind.yml, and verifying that we get sccache hits on try. For reference, taskcluster/ci/build/windows.yml has sccache enabled for most Windows builds.

Comment 2

[Ricky Stewart]

Attachment: Fix Bug 1583368. Don't use sccache on dependencies of shippable builds

Comment 3

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1f7e1be8adb92a48a2dd5cea164c01bf17ed56a

Comment 4

[Tom Prince [:tomprince]]

I do not think we can have sccache enabled for builds used in the release process (which instrumented builds are). sccache uses S3 (or gcs) to store artifacts, and we don't have any way to verify that those artifacts aren't modified between creation and use (or to verify that they were ever created by a trusted build).

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Morphing bug based on comment 4. Tom also pointed me at https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/taskcluster/taskgraph/util/verify.py#235-240 which should be adjusted to catch things like this.

Comment 6

[Ricky Stewart]

Attachment: Fix Bug 1583368. Don't use sccache on dependencies of shippable builds

Comment 7

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5859d076074db0bc733b5dfa1a1876b61462d134

Comment 8

[Michael Shal [:mshal]]

:hwine, can you provide some guidance on how to proceed here and land this? Presumably we'd also want to uplift to beta & release.

Comment 9

[Michael Shal [:mshal]]

:catlee, can you comment on how this might impact the overall TC load? We'd be turning off sccache on the Linux & Android instrumented ("instr") builds. Historically, we didn't have sccache on the nightly PGO builds, but I think we did for the non-nightly PGO builds. Now, shippable builds and any non-shippable PGO builds use the profile data derived from the same instrumented builds, so they need to not use sccache.

Comment 10

[Hal Wine [:hwine] use NI!]

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

:hwine, can you provide some guidance on how to proceed here and land this? Presumably we'd also want to uplift to beta & release.

I think the following 2 steps should be sufficient to land:

• check with RelEng on any potential load/performance issues and any uplift criteria they have (comment 9 addresses this)
• get a code review from RelEng and/or Taskcluster that the patch addresses all the cases we're concerned about.

Comment 11

[Chris AtLee [:catlee]]

I suspect this will result in a marginal but acceptable increase in load. We could mitigate that by only disabling sccache on repositories we ship from. i.e. could we leave sccache enabled for shippable builds on try and autoland?

Comment 12

[Tom Prince [:tomprince]]

Note that this is not a regression from 1-tier PGO, since we weren't running sccache with shippable builds then.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/20516e38f03595cfee7e3533d96fbf8b1db934b2
https://hg.mozilla.org/mozilla-central/rev/20516e38f035

Comment 14

[Mike Hommey [:glandium]]

I want to ask why? What's the risk of the instrumented builds having their build cache tempered with? The only one I can think of is getting suboptimal optimizations as a result.

Comment 15

[Michael Shal [:mshal]]

Comment on attachment 9094762 [details]
Fix Bug 1583368. Don't use sccache on dependencies of shippable builds

Beta/Release Uplift Approval Request

• User impact if declined: No impact to end users.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change just disables sccache on instrumented builds. The builds work fine without sccache, they just take longer to complete.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: No impact to end users.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 16

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #14)

I want to ask why? What's the risk of the instrumented builds having their build cache tempered with? The only one I can think of is getting suboptimal optimizations as a result.

As I understand it, the risk is that a specific object file could be crafted and injected into the cache, such that when the instrumented Firefox binary runs, it generates malformed profile data that triggers an exploit in clang to cause the final profile-use build to contain arbitrary code. Without such an exploit in clang, the risk then just becomes suboptimal optimizations.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9094762 [details]
Fix Bug 1583368. Don't use sccache on dependencies of shippable builds

Doesn't affect the builds we ship in the end, just mitigates a possible opportunity for bad actors to compromise them via sccache. Approved for all the things.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b68b1900d1e8
https://hg.mozilla.org/releases/mozilla-release/rev/3cc6aa4566a2
https://hg.mozilla.org/releases/mozilla-esr68/rev/feeabad9e3f1