Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chromium: 60.0.3112.90 -> 61.0.3163.79 [security] #29335

Merged
merged 5 commits into from Sep 16, 2017

Conversation

bendlas
Copy link
Contributor

@bendlas bendlas commented Sep 13, 2017

Motivation for this change

security update, please backport

CVE-2017-5111
CVE-2017-5112
CVE-2017-5113
CVE-2017-5114
CVE-2017-5115
CVE-2017-5116
CVE-2017-5117
CVE-2017-5118
CVE-2017-5119
CVE-2017-5120

includes #28857

Things done

Building right now, at about 60%

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@bendlas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @aszlig, @abbradar and @domenkozar to be potential reviewers.

@bendlas
Copy link
Contributor Author

bendlas commented Sep 14, 2017

Force pushed with another patch, necessary for building. Build at 80% now.

@vcunat
Copy link
Member

vcunat commented Sep 14, 2017

With the two commits atop 17.09, the builds failed for me at the very end:

cp: missing destination file operand after '/nix/store/xh6cymx3facbnr8r932cl7ynbl7m8hwa-chromium-61.0.3163.79/libexec/chromium/swiftshader/'
Try 'cp --help' for more information.
builder for ‘/nix/store/j1fhcws9hflg2dsam2zczg0k095fmv42-chromium-61.0.3163.79.drv’ failed with exit code 1
cannot build derivation ‘/nix/store/w214xwrpaffvaqr6wgy8bdcn4bgdsbjg-chromium-61.0.3163.79.drv’: 1 dependencies couldn't be built

@bendlas
Copy link
Contributor Author

bendlas commented Sep 15, 2017

swiftshader apparently got integrated recently. trying a build without swiftshader, as per latest patch ...

@FRidh FRidh requested a review from aszlig September 15, 2017 11:04
@bendlas
Copy link
Contributor Author

bendlas commented Sep 15, 2017

pushed another installPhase fix, running another build ...

Copy link
Member

@aszlig aszlig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove that /chromium from mkdir -p, otherwise it looks fine.

@@ -12,11 +12,10 @@ mkChromiumDerivation (base: rec {
sandboxExecutableName = "__chromium-suid-sandbox";

installPhase = ''
mkdir -p "$libExecPath/swiftshader"
mkdir -p "$libExecPath/chromium"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just $libExecPath is enough, because it already contains chromium, the reason this was $libExecPath/swiftshader is that mkdir -p already creates parent directories, which then includes $out/libexec/chromium.

Copy link
Contributor Author

@bendlas bendlas Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my last build, I had shortened it to mkdir -p "$libExecPath", and the installPhase broke due to that directory missing. I changed it to this version, nix-shelled into the --keep-failed chromium build, ran out=/tmp/out; sandbox=/tmp/sandbox eval "$configurePhase"; eval "$buildPhase"; eval "$installPhase"; fixupPhase and got, what looked like a good build in /tmp/out. I'm re-running build now, at [21185/27987] to verify. I'll probably check back tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh .. I only saw already contains chromium now .. that seems fishy ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, I probably interpreted the build error wrong, and it actually broke because of the spurious , I had left in before: 209b9e8#diff-69a3af1d07b37077e7dd11ccc8f9b61eR15

cp -v "$buildPath/"*.pak "$buildPath/"*.bin "$libExecPath/"
cp -v "$buildPath/icudtl.dat" "$libExecPath/"
cp -vLR "$buildPath/locales" "$buildPath/resources" "$libExecPath/"
cp -v "$buildPath/swiftshader/"*.so "$libExecPath/swiftshader/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this no longer needed? Does WebGL work for you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry... missed your comment about swiftshader being integrated, nevertheless, please try if WebGL works, because we had that broken a few weeks ago already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that cp broke the build (because the source folder was empty), so I removed it. in research, I found that thing about swiftshader being integrated and it figured. I'll try WebGL, as soon as the build finishes.

@bendlas
Copy link
Contributor Author

bendlas commented Sep 15, 2017

pushed fix as per feedback

starting another build ...

Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-tested the default version atop 17.09. All seems OK - including webGL demos/tests.

vcunat added a commit that referenced this pull request Sep 16, 2017
@vcunat vcunat merged commit 6141d8e into NixOS:master Sep 16, 2017
@joachifm
Copy link
Contributor

Nice :)

@vcunat
Copy link
Member

vcunat commented Sep 16, 2017

Hmm, I tried to port this to 17.03 but I failed. WIP: https://github.com/vcunat/nixpkgs/commits/p/chromium-17.03

It's possible that the commit works and I'm just repeatedly hitting the Ryzen bug in the particular case. You can try on your machine.

@nh2
Copy link
Contributor

nh2 commented Jun 11, 2018

@bendlas @aszlig I think this change is breaking enable_swiftshader = true; support:

-cp -v "$buildPath/swiftshader/"*.so "$libExecPath/swiftshader/"

It looks like the swiftshader-related .so files are only generated when enable_swiftshader = true;, that's why you had to remove the cp (it defaults to false).

But now swiftshader files are never copied, even when they are generated. So headless WebGL in Chromium is broken on NixOS right now.

I have a build running to confirm whether it is as I say.

@bendlas
Copy link
Contributor Author

bendlas commented Jun 12, 2018

Interesting. That must have been the time, the flag was defaulted to false. I deleted those lines, because the .so files had vanished from the build and made the cp fail. In hindsight, it would have made more sense to insert a conditional instead, or even look for the flag ...

Thanks for digging this one out. I hope your build succeeds!

@nh2
Copy link
Contributor

nh2 commented Jun 13, 2018

I have a build running to confirm whether it is as I say.

@bendlas Can confirm, the build succeeds with enable_swiftshader = true; and cp -v "$buildPath/swiftshader/"*.so "$libExecPath/swiftshader/" reintroduced, and this makes SwiftShader based WebGL work again.

The graphics look a little off/corrupted when swiftshader is used vs my normal GPU, but that's an upstream problem, not a nixpkgs problem.

So I think we should thange the derivation to copy exactly when enable_swiftshader is given.

I'll file a new issue to track this (edit: #41918).

@samueldr samueldr removed the 9.needs: port to stable A PR needs a backport to the stable release. label Apr 17, 2019
@nh2 nh2 mentioned this pull request Mar 24, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants