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

fetchzip: remove write permissions for unpacked files #102114

Merged
merged 1 commit into from Nov 29, 2020

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented Oct 30, 2020

Motivation for this change

Fixes #38649

Some zip files contain 4.4.15 external file attributes that may set the write bit of unpacked files. This change ensures that all files fetched & unpacked with fetchzip will have their write bits cleared as final step. This change should not affect the hash of any outputs built with fetchzip.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
    I manually rebuilt all projects that used extraPostFetch as a workaround, and they all still resolve to the their original hashes. However, ipmicfg isn't exposed through an attribute, and when I tried to build it, it failed with:
    End-of-central-directory signature not found
    
    Should it just be removed?
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@kira-bruneau kira-bruneau changed the title fetchzip: Remove write permissions for unpacked files fetchzip: remove write permissions for unpacked files Oct 30, 2020
@SuperSandro2000
Copy link
Member

Can you fix the merge conflict?

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Can someone clarify if this does not break anything?

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 26, 2020

Can you fix the merge conflict?

Yep, thank you. I just submitted a fix now.

Can someone clarify if this does not break anything?

Anything that currently builds with fetchzip doesn't have write permissions set, so this change makes no difference to anything that built succesfully before. This only affects zips that have write permissions set, which previously failed to build without patching.

I've tested this change on a few projects, including all the projects that previously needed to be patched, but I'm not sure of a good way to test everything other than setting all hashes passed to fetchzip to something invalid and then redownloading everything.

Even if it did change the hash, which it definitely shouldn't, the previous hash would be cached and would be used instead.

@kira-bruneau
Copy link
Contributor Author

Result of nixpkgs-review pr 102114 1

1 similar comment
@bhipple
Copy link
Contributor

bhipple commented Nov 29, 2020

Result of nixpkgs-review pr 102114 1

@bhipple bhipple merged commit 9426084 into NixOS:master Nov 29, 2020
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.

fetchzip does not set ownership / permissions on $out
3 participants