discord: add Krisp patcher#506089
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
34cbd38 to
53d44f0
Compare
This comment has been minimized.
This comment has been minimized.
|
does this go against Discord's TOS? from the wiki regarding patching the binary:
if so, should this be in the official NixOS packages? EDIT: totally missed it's off by default, my bad |
By this logic that would mean we would have to drop Vencord and Equicord and all other 3rd party Discord clients, as Discord doesn't allow any 3rd party clients or modifications Practically nobody has ever been banned for using 3rd party clients or modifications (when used in "good faith"), Discord seemingly only cares that you don't hammer their API (which obv none of these stuff do) EDIT: I re-read it again and as well I saw your edit, seems like we misunderstood each other, to clarify it's disabled by default ofc, and I said in PR itself, it's enabled implicitly by using 3rd party modification like Vencord or Equicord as you're knowingly breaking ToS anyways |
This comment has been minimized.
This comment has been minimized.
53d44f0 to
d87fa33
Compare
|
@ashuramaruzxc Thanks a lot for the review! |
ashuramaruzxc
left a comment
There was a problem hiding this comment.
Some parts of the code regarding reading files and bytes could be improved but overall lgtm
This comment has been minimized.
This comment has been minimized.
1c52ef9 to
661d16d
Compare
|
Now that #507728 is merged, this PR needed more than just conflict resolution That PR changed how the non-stable Linux Discord packages are fetched/staged: PTB/Canary/Development now use Discord's distro/module packaging, and I updated this PR to support both worlds: legacy Krisp zips and distro module Krisp sources. For distro modules, Krisp can be a brotli-compressed tar module, so the patching path handles that too. When For the relevant changes please look at: 1c52ef9 (I squashed it out because I wanted it to purely serves as a clean diff) |
661d16d to
3fd9c5c
Compare
This comment has been minimized.
This comment has been minimized.
|
This heavily leaks memory on my system. Just leaving Discord open for a couple of hours with this enabled, the Python script ends up consuming around ~40 GB of RAM. |
This might not be related to this. I’m experiencing a similar issue when screen sharing is active while using Vesktop. I don’t think this is related to this PR. |
No, it's specifically the Python script added in this PR, which I've pulled in as a patch against my nixpkgs. |
Please provide your system information, I've tried keeping discord open with this patch already over 30 hours and it doesn't seem to leak memory with this patch on both x86_64-linux and aarch64-darwin |
3fd9c5c to
5784401
Compare
I couldn't really replicate this on my side, but I added a fix for what looks like the likely cause. Even aside from the memory issue, this should be the more correct watcher behavior |
|
It seems to be fixed on my end |
5784401 to
3327261
Compare
|
Can confirm this fixed my issues. |
|
koffydrop
left a comment
There was a problem hiding this comment.
Approved automatically following the successful run of nixpkgs-review.
|
Krisp is Discord's proprietary noise cancellation feature. On nixpkgs it fails to load because patchelf/code signing changes cause the module's built-in signature verification to fail (see #195512)
This PR binary-patches
discord_krisp.nodeto bypass the signature check, deploys the patched module to the user's Discord config dir at runtime, and handles macOS ad-hoc re-signingThere was a prior attempt (and this was inspired by it):
objdump. This approach technically works. LocatingIsSignedByDiscordby its mangled symbol and patching it is functionally equivalent to what we do here.The problem is maintenance: If the symbol gets stripped/renamed entirely (which Discord could do at any time), the approach breaks with no fallback. Also see #290077 (review)
Here is how my patch works (conceptually):
On ELF, the patcher finds a unique byte pattern in
.textthat corresponds to the MD5 comparison in the signature check (pmovmskb+cmp $0xffff), walks back to the function start, and overwrites it withmov eax, 1; retOn Mach-O (macOS), it starts from the
_SecStaticCodeCreateWithPathimport stub and walks callers upward, each hop is the single unique caller of the previous target. When the chain fans out (multiple callers), we've foundIsSignedByDiscord()and patch it to return true. Then re-sign withdarwin.signingUtilsAt runtime,
deploy-krisp.pycopies the patched module into the user's config dir before Discord starts. It uses a SHA-256 marker to skip redeployment on subsequent launches and to catch cases where vanilla Discord overwrites our files (macOS APFS case-insensitivity). On fresh installs it waits for Discord to bootstrap its core modules before writing toinstalled.jsonI'm keeping it off by default tho, using Krisp with a modified Discord client probably violates Discord's ToS. But, using Vencord/Equicord/Moonlight already violates those same terms, so enabling Krisp alongside them doesn't introduce any new ToS risk
Closes #195512
Tested on NixOS and Nix-Darwin
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.