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.

Sign up for GitHub

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

Jump to bottom

ffmpeg-3.4: apply fix CVE CVE-2017-16840 #32126

Merged
merged 2 commits into from Nov 28, 2017
Merged

ffmpeg-3.4: apply fix CVE CVE-2017-16840 #32126

merged 2 commits into from Nov 28, 2017

Conversation

andir
Copy link
Member

@andir andir commented Nov 27, 2017

Details at [1].

[1] http://git.videolan.org/?p=ffmpeg.git;a=commit;h=a94cb36ab2ad99d3a1331c9f91831ef593d94f74

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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.

@andir
Copy link
Member Author

andir commented Nov 27, 2017

@GrahamcOfBorg seems to fail due to bfc0959 causing

error: attribute ‘homepage’ missing, at /home/andi/…/nixpkgs/pkgs/development/ocaml-modules/cstruct/default.nix:21:10

@grahamc
Copy link
Member

grahamc commented Nov 28, 2017

@GrahamcOfBorg eval

@vcunat
Copy link
Member

vcunat commented Nov 28, 2017

fetchurl in one case, fetchpatch in the other one and the same hashes?

@vcunat
Copy link
Member

vcunat commented Nov 28, 2017

BTW, I think we would better convert such usages of patchPhase to prePatch or postPatch, so that the usual patches attribute works as well.

@andir
Copy link
Member Author

andir commented Nov 28, 2017

The sha256 should be the same in both cases since the input file is really the same. It did compile in both cases for me locally.

The ffmpegFull version is using fetchurl since it doesn't really use the "standard" patches approach so I thought that might be suited there. The name attribute is a left-over from moving the patch from ffmpeg to ffmpegFull.

I agree that the patchPhase should probably not be used like that.

I'll update the PR to convert the "normal" patching into prePatch and use the same fetchpatch there was with the ffmpeg package.

@vcunat
Copy link
Member

vcunat commented Nov 28, 2017

The point of fetchpatch is that it normalizes the patch, so the output hash shouldn't be the same.

@vcunat vcunat self-assigned this Nov 28, 2017
@andir
Copy link
Member Author

andir commented Nov 28, 2017

Thank you for pointing that out and explaining it (as always 👍 )! I just had a look at the fetchpatch implementation... makes sense.

vdemeester pushed a commit to vdemeester/nixpkgs that referenced this pull request Nov 28, 2017
@vcunat
Merge NixOS#32126: ffmpeg-3.4: fix CVE CVE-2017-16840
c917950
vcunat added a commit that referenced this pull request Nov 28, 2017
@vcunat
Merge #32126: ffmpeg-3.4: fix CVE CVE-2017-16840
db0bb7f 
(cherry picked from commit c917950)
@vcunat vcunat merged commit fe1f228 into NixOS:master Nov 28, 2017
vcunat added a commit that referenced this pull request Nov 28, 2017
@vcunat
Re-merge #32126: ffmpeg-3.4: fix CVE CVE-2017-16840
fac570a 
I'm sorry, I merged older version of the PR by accident.
vcunat added a commit that referenced this pull request Nov 28, 2017
@vcunat
Re-merge #32126: ffmpeg-3.4: fix CVE CVE-2017-16840
9baaf3a 
I'm sorry, I merged older version of the PR by accident.

(cherry picked from commit fac570a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vcunat vcunat vcunat approved these changes

Assignees

@vcunat vcunat

Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andir @grahamc @vcunat @GrahamcOfBorg