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

patchutils: various fixes #53449

Closed
wants to merge 4 commits into from
Closed

Conversation

timokau
Copy link
Member

@timokau timokau commented Jan 5, 2019

Motivation for this change

Some of the patchutils scripts weren't working because the perl-shebang wasn't generated properly. When that was fixed, they still didn't work because they depended on each other.

This fixes those issues by adding perl to build inputs and wrapping the scripts with PATH. While at it, I also update patchutils to the latest version which makes the patch unnecessary and fixed the tests.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@timokau
Copy link
Member Author

timokau commented Jan 5, 2019

Since there is no maintainer, based on git shortlog:

@MarcWeber @globin @dezgeg @vcunat

@timokau
Copy link
Member Author

timokau commented Jan 5, 2019

I would have expected way more rebuilds, but I guess I forgot that fetchpatch is fixed output. Will rebase on master.

@timokau
Copy link
Member Author

timokau commented Jan 5, 2019

But then again maybe the fetchpatch output changed somehow with the update :/

@globin
Copy link
Member

globin commented Jan 5, 2019

Yes, please check some fetchpatch hashes, as I think I remember we had issues previously.

@timokau
Copy link
Member Author

timokau commented Jan 5, 2019

Looks like they did actually change

@timokau
Copy link
Member Author

timokau commented Jan 5, 2019

Apparently the new patches miss these two lines at the top:

diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c
index 26dc39a41..c5065bc97 100644

@timokau
Copy link
Member Author

timokau commented Jan 6, 2019

This is the offending comment: twaugh/patchutils@14261ad

It fixes the header detecton and makes sure extended git headers are not dropped as they were before (apparently by accident). I got it wrong in my previous comment: the new patch does not miss those lines, while the previous ones did.

Not sure what to do here. Revert the commit? Never upgrade patchutils? Adjust all hashes in nixpkgs somehow?

@vcunat
Copy link
Member

vcunat commented Jan 6, 2019

This isn't the first time at all: #25154 (and links)

@timokau
Copy link
Member Author

timokau commented Jan 6, 2019

Interesting, thanks for the link. I personally don't care very much for the update, so I'd be fine with leaving that out. But then I'll at least add a comment to avoid duplicate work in the future.

What is your preference? Keep it at 0.3.3, have both 0.3.3 and 0.3.4 or update to 0.3.4 while reverting that one commit?

@vcunat
Copy link
Member

vcunat commented Jan 6, 2019

Well, ATM it's probably not worth it to change all the hashes in nixpkgs (I haven't looked at their changes though). Over longer term it might be best to split the version we use for fetchpatch and be careful about that one's update, while changing the other one rather freely.

@timokau
Copy link
Member Author

timokau commented Jan 6, 2019

patchutils is somewhat security relevant to nix though isn't it? If there were a security issue allowing for example remote code execution on patch download, somebody could modify a patch file nixpkgs links to. Since the sha256 is only verified after patchutils was run (right?), that may be an issue.

@vcunat
Copy link
Member

vcunat commented Jan 6, 2019

Right, I agree that even for fetchpatch we should at least patch this CVE; another option there would be full update with that content-changing patch reverted.

@timokau
Copy link
Member Author

timokau commented Jan 6, 2019

That was a security issue in patch itself though right? It looks like that is already fixed in our gnupatch package.

@vcunat
Copy link
Member

vcunat commented Jan 8, 2019

Right, I believe so. I got confused by a debian thread.

@vcunat
Copy link
Member

vcunat commented Jan 8, 2019

0.3.3 -> 0.3.4 doesn't seem to be relevant for security.

@jameysharp
Copy link
Contributor

I just ran into these bugs today, so 👍 to fixing them. Thanks for investigating how to fix it!

@mmahut
Copy link
Member

mmahut commented Aug 20, 2019

Are there any updates on this pull request, please?

@ckauhaus
Copy link
Contributor

@timokau What is the state of this PR? It should at least be rebased to current master and squashed.

@timokau
Copy link
Member Author

timokau commented Nov 24, 2019

IIRC the issue is that it will change all the fetchpatch hashes, and I know of no reasonable way to update them all.

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@ajs124
Copy link
Member

ajs124 commented Jun 1, 2020

How about merging the other changes, without the version bump, since some people seem to have run into issues without them?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@timokau
Copy link
Member Author

timokau commented Jun 1, 2020

@ajs124 I don't plan to work on this any time soon. I'd be happy to review if you want to take this over though.

@stale
Copy link

stale bot commented Jun 9, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 9, 2021
@Artturin
Copy link
Member

#141567

@Artturin Artturin closed this Oct 13, 2021
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

10 participants