jesec-rtorrent: Add patch to prevent segfault

[vamega]

Description of changes

I've submitted the attached patch upstream. jesec/rtorrent#67
This prevents a core dump that started to happen recently (I updated last about a week ago).

As mentioned, the original patch is from: rakshasa/rtorrent#1169.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[AndersonTorres]

Will this patch be upstreamed?

[AndersonTorres]

Suggested change

	./avoid-stack-overflow-for-lockfile-buf.patch
	./000-avoid-stack-overflow-for-lockfile-buf.patch

[winterqt]

This isn't even a convention we use, though... why waste maintainer bandwidth on such a tiny change? (Also: see below, fetchpatch could have been used instead.)

[vamega]

I’ve submitted a PR to the upstream project, this patch is from that PR.

I expect it will be accepted, as the equivalent was accepted into rakshasa-rtorrent. If it isn’t I’ll post an update here.

[winterqt]

@AndersonTorres I'd appreciate it if you'd have waited until this PR has been open for at least 24 hours, so I could have seen this and suggested a worthwhile change (using fetchpatch, as the upstream commit should have applied cleanly).

I don't do this to packages I maintain with someone else, so I'd appreciate it if you gave me the same courtesy.

[vamega]

The upstream PR was changed slightly so as to preserve author information.
Once it's merged upstream I'll submit a PR with fetchPatch and the upstream commit until a new upstream release is cut.

[winterqt]

There's a better way to do this even now, @vamega. I'm going to test + make a cleaner PR.

[vamega]

Just to be clear, you mean a cleaner PR upstream, or just to nixpkgs?
If you're going to submit something upstream, I'll note that my PR shouldn't be merged just yet.

[winterqt]

Just to Nixpkgs :)

[AndersonTorres]

Sorry, my mistake.
I originally supposed jesec was built from unstable git, not from a tagged release. My idea was just release this and fetch later after upstream merge.

[vamega]

The patch landed upstream.

[AndersonTorres]

Time to fetch unstable, then!

[winterqt]

There's no point in doing that... I need to sort out a few things, then I'll PR a clean up for this.

[winterqt]

We really shouldn't rely on unstable versions if we can get away with not doing it... (which in this case, for a one line patch, we absolutely can).

😕

[winterqt]

So, a few things:

It turns out that the original upstream patch (rakshasa/rtorrent@92bec88) can't be cleanly applied to jesec's tree (or at least, the tag we pull now) as I thought, so this route was probably the best to go at the time.

@vamega, thank you for the PR, and apologies that your notifications have blown up a tad from me here. Out of curiosity, did this issue just manifest in unstable? I can't reproduce from a Nixpkgs version in early July, so I imagine we bumped GCC or toggled some hardening setting (as mentioned in the upstream PR) that caused this to manifest. Nonetheless, thank you for spotting this and fixing it.

@AndersonTorres, while I appreciate the quick merge fixing (from what I can tell) a pretty major issue, I'd rather you at least wait for me to review it as a co-maintainer of the package. I admit I'm not the easiest person to get a hold of nowadays, most of which is my fault, but I'm going to try to fix my inbox a bit/curb my GitHub notifications so things that need to reach me do so promptly. But please do at least give me 24 hours or so to respond 🙁

I also don't really understand your push to want to fetch an unstable version of rTorrent -- as software that could go deleting a ton of files that people definitely don't want deleted, I'm not a fan of the prospect of pushing an unstable tag unless we really have to. I'd appreciate it if you could explain your thought process regarding that, just for my understanding if nothing else.

Happy to clarify any of the above if needed, just let me know. Thanks.

[AndersonTorres]

Nothing special. I just thougth it would be easier to fetch the git head of jesec-rtorrent than to fetch a patch.

[vamega]

@winterqt, yes this issue did appear in unstable. I saw a core dump and the service failed to start. I knew the original patch couldn't cleanly be applied, which is why I had taken this approach.

The jesec upstream has now accepted a patch that preserves the original authorship. It's probably possible to fetch and apply https://github.com/jesec/rtorrent/commit/8c426e55e2adb3a4a81239188e8a260e8e163106.patch if we want to use a tagged version until a new release is tagged.