discord: fix krisp module by jopejoe1 · Pull Request #290077 · NixOS/nixpkgs

jopejoe1 · 2024-02-20T00:34:21Z

Description of changes

This includes a modified version of @sersorrel's krips-patcher.py to fix krisp not working.

If it's okay with you @sersorrel that we included it in nixpkgs.

Closes #195512

Things done

Add a 👍 reaction to pull requests you find important.

Artturin · 2024-02-20T20:15:51Z

pkgs/applications/networking/instant-messengers/discord/linux.nix

    substituteAllInPlace $out/bin/disable-breaking-updates.py
    chmod +x $out/bin/disable-breaking-updates.py
  '';
+  fixKrisp = runCommand "fix-krisp.py"


add to passthru

keysmashes

I have no objections to this getting upstreamed. I did not in fact write krisp-patcher.py, though – that was @Stary2001 – so it's not really my call.

I don't expect the patcher part to need much maintenance; I had to make one change (tweaking a symbol name) in the last year or more.

I am slightly dubious about the approach. I realise that in the normal case, this will run discord by hand for exactly long enough to download krisp, then kill it, apply the patch, and let the normal wrapper script launch it again, but in the case where the patcher doesn't see installed-module discord_krisp – maybe one day discord decides to change their logging format – then the getKrisp discord instance will presumably run forever.
Then, until someone notices that krisp is broken and figures out what to do about it, it seems entirely possible that anyone relying on things like the --enable-speech-dispatcher flag that the wrapper script adds will be very confused about why their TTS isn't working, even though they definitely enabled withTTS.

keysmashes · 2024-02-20T21:33:24Z

pkgs/applications/networking/instant-messengers/discord/fix-krisp.py

+from capstone.x86 import *
+
+def getKrisp():
+    process = subprocess.Popen(sys.argv[1], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, shell=False, universal_newlines=True, preexec_fn=os.setsid)


stderr=subprocess.STDOUT seems dubious – presumably this should search either stdout or stderr, whichever this log goes to, and pass the other one through?

shell=False is the default

I would be inclined to drop universal_newlines=True and search for bytes in the output instead (b"installed-module ..."), but shrug

pass start_new_session=True rather than using preexec_fn=os.setsid

keysmashes · 2024-02-20T21:40:34Z

pkgs/applications/networking/instant-messengers/discord/fix-krisp.py

+    try:
+        for line in iter(process.stdout.readline, ''):
+            print(line, end='')
+            if re.search("installed-module discord_krisp", line):


re.search doesn't seem necessary given the needle is a fixed string – would if "..." in line be sufficient?

keysmashes · 2024-02-20T21:49:27Z

pkgs/applications/networking/instant-messengers/discord/fix-krisp.py

+
+krisp_initialize_address = symtab.get_symbol_by_name("_ZN7discord15KrispInitializeEv")[0].entry.st_value
+isSignedByDiscord_address = symtab.get_symbol_by_name("_ZN7discord4util17IsSignedByDiscordERKNSt2Cr12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE")[0].entry.st_value


a note about how these were derived could be useful in the future:

Suggested change

krisp_initialize_address = symtab.get_symbol_by_name("_ZN7discord15KrispInitializeEv")[0].entry.st_value

isSignedByDiscord_address = symtab.get_symbol_by_name("_ZN7discord4util17IsSignedByDiscordERKNSt2Cr12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE")[0].entry.st_value

# see e.g. `objdump -t | grep IsSignedByDiscord`

krisp_initialize_address = symtab.get_symbol_by_name("_ZN7discord15KrispInitializeEv")[0].entry.st_value

isSignedByDiscord_address = symtab.get_symbol_by_name("_ZN7discord4util17IsSignedByDiscordERKNSt2Cr12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE")[0].entry.st_value

keysmashes · 2024-02-20T21:50:33Z

pkgs/applications/networking/instant-messengers/discord/fix-krisp.py

+
+executable = f"{XDG_CONFIG_HOME}/@configDirName@/@version@/modules/discord_krisp/discord_krisp.node"
+
+if (not os.path.exists(Path(executable))):


Suggested change

if (not os.path.exists(Path(executable))):

if not Path(executable).exists():

Stary2001 · 2024-02-20T23:00:15Z

I got mentioned in a review comment - I wrote the initial version of that patching script, I'm ok with it being included under whatever license.

jopejoe1 · 2024-02-22T00:32:19Z

Need to rethink my approach to fix this. The more I think about, the flakier it looks.

keysmashes · 2024-03-23T17:08:45Z

Just a note that the isSignedByDiscord_address symbol has changed to _ZN7discord4util17IsSignedByDiscordERKNSt4__Cr12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE,

keysmashes · 2025-03-30T09:33:32Z

@nezia1 I would gently suggest that discussion about this PR happen on this PR (particularly since I can't reply to your discord message, for unclear reasons).

nezia1 · 2025-03-30T11:02:23Z

@nezia1 I would gently suggest that discussion about this PR happen on this PR (particularly since I can't reply to your discord message, for unclear reasons).

I'm okay with that. As the PR went stale, I've been looking to pick it back up especially considering the amount of work that has been done from your side and from @jopejoe1's.

I've mostly been wondering about the values in the krisp patcher script; specifically the long strings of characters that seem derived from something:

Could those be automatically re-calculated on each new release, or would they need to be manually sourced?
How exactly are they derived? I saw the comment but I'm not sure what we're running objdump on.

keysmashes · 2025-03-30T11:35:39Z

Could those be automatically re-calculated on each new release, or would they need to be manually sourced?

They don't change much (I haven't needed to update the script since October last year), but when they do it will typically need human intervention.

How exactly are they derived? I saw the comment but I'm not sure what we're running objdump on.

They can change for two reasons:

the mangling changed (see https://demangler.com/ for an explanation; for example, _ZN7discord4util17IsSignedByDiscordERKNSt2Cr12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE is a mangled version of discord::util::IsSignedByDiscord(std::Cr::basic_string<char, std::Cr::char_traits<char>, std::Cr::allocator<char> > const&))
the structure of the program changed

The former is usually relatively easy to fix: run objdump -t | grep KrispInitialize or objdump -t | grep IsSignedByDiscord on the Krisp binary and find the new names. (The binary is located at e.g. ~/.config/discord/0.0.88/modules/discord_krisp/discord_krisp.node; the version number changes frequently.)

The latter can be more difficult to fix and requires inspecting the disassembled binary to see how to patch out the signature check, usually using a tool like Ghidra or Binary Ninja.

nezia1 · 2025-12-02T12:07:49Z

Hey, I looked at #424232 and it seemed to be a way more maintainable solution to our problem. Wdyt @keysmashes? It'd allow us to never worry about updating the symbols

discord: fix krisp module

f8d61c3

ofborg bot requested review from Artturin, Infinidoge, MP2E and Scrumplex February 20, 2024 01:08

ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 20, 2024

Artturin reviewed Feb 20, 2024

View reviewed changes

keysmashes reviewed Feb 20, 2024

View reviewed changes

jopejoe1 marked this pull request as draft February 22, 2024 00:32

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 30, 2025

nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 25, 2025

wolfgangwalther mentioned this pull request Jun 25, 2025

Reapply "workflows/labels: manage stale & merge conflict labels" #419654

Merged

2 tasks

nixpkgs-ci bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 25, 2025

nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 27, 2025

nixpkgs-ci bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 2, 2025


		krisp_initialize_address = symtab.get_symbol_by_name("_ZN7discord15KrispInitializeEv")[0].entry.st_value
		isSignedByDiscord_address = symtab.get_symbol_by_name("_ZN7discord4util17IsSignedByDiscordERKNSt2Cr12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE")[0].entry.st_value


		executable = f"{XDG_CONFIG_HOME}/@configDirName@/@version@/modules/discord_krisp/discord_krisp.node"

		if (not os.path.exists(Path(executable))):

	if (not os.path.exists(Path(executable))):
	if not Path(executable).exists():

Uh oh!

Conversation

jopejoe1 commented Feb 20, 2024

Description of changes

Things done

Uh oh!

Artturin Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

keysmashes left a comment

Choose a reason for hiding this comment

Uh oh!

keysmashes Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

keysmashes Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

keysmashes Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

keysmashes Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

Stary2001 commented Feb 20, 2024

Uh oh!

jopejoe1 commented Feb 22, 2024

Uh oh!

keysmashes commented Mar 23, 2024

Uh oh!

keysmashes commented Mar 30, 2025

Uh oh!

nezia1 commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keysmashes commented Mar 30, 2025

Uh oh!

nezia1 commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nezia1 commented Mar 30, 2025 •

edited

Loading