jackpatch crashing at load (buffer overflow)

[ventosus]

This happens when jackpatch is run under pipewire-jack.

REAL_JACK_PORT_NAME_SIZE may not have been initialized, yet?

Version

$ jackpatch --version
1.0.0

Invocation

$ jackpatch JACKPatch.nBKXL.jackpatch 
[jackpatch] Reading connections from file JACKPatch.nBKXL.jackpatch 
*** buffer overflow detected ***: terminated
Aborted (core dumped)

GDB dump

#8  0x000055555555697c in snprintf (__fmt=0x55555555849f "%s:%s", __n=<optimized out>, 
  __s=0x7fffffffda70 "*\202") at /usr/include/bits/stdio2.h:54
#9  connect_path (pr=0x55555559da40) at ../new-session-manager-1.6.1/src/jackpatch.c:334
#10 0x0000555555556b07 in do_for_matching_patches (
  portname=portname@entry=0x7ffff0008208 "Komplete Audio 6 Pro:playback_AUX0", 
  func=func@entry=0x555555556920 <connect_path>) at ../new-session-manager-1.6.1/src/jackpatch.c:398
#11 0x0000555555556c74 in activate_patch (portname=0x7ffff0008208 "Komplete Audio 6 Pro:playback_AUX0")
  at ../new-session-manager-1.6.1/src/jackpatch.c:418
#12 handle_new_port (portname=0x7ffff0008208 "Komplete Audio 6 Pro:playback_AUX0")
  at ../new-session-manager-1.6.1/src/jackpatch.c:461
#13 register_prexisting_ports () at ../new-session-manager-1.6.1/src/jackpatch.c:475
#14 0x0000555555556380 in main (argc=2, argv=0x7fffffffe268)
  at ../new-session-manager-1.6.1/src/jackpatch.c:983

Config (JACKPatch.nBKXL.jackpatch)

Midi-Bridge:FH-2 3:(capture_0) FH-2 MIDI 1 |> Synthpod-JACK.nTAKF:#130618_midi_sink
Midi-Bridge:Launchpad Mini MK3 4:(capture_1) Launchpad Mini MK3 LPMiniMK3 MI |> Synthpod-JACK.nTAKF:#40483_midi_sink
Midi-Bridge:Native Instruments Komplete Audio 6 at usb-0000:01:00-0-1- high speed:(capture_0) Komplete Audio 6 MIDI 1 |> Synthpod-JACK.nTAKF:#4961_midi_sink
Synthpod-JACK.nTAKF:#53477_midi_source   |> Midi-Bridge:FH-2 3:(playback_0) FH-2 MIDI 1
Synthpod-JACK.nTAKF:#73784_midi_source   |> Midi-Bridge:Launchpad Mini MK3 4:(playback_1) Launchpad Mini MK3 LPMiniMK3 MI
Synthpod-JACK.nTAKF:#77687_audio_source_1 |> Komplete Audio 6 Pro:playback_AUX0
Synthpod-JACK.nTAKF:#77687_audio_source_2 |> Komplete Audio 6 Pro:playback_AUX1

[ventosus]

This fixes the issue for me:

diff --git a/src/jackpatch.c b/src/jackpatch.c
index bc9303b..c5d6ec9 100644
--- a/src/jackpatch.c
+++ b/src/jackpatch.c
@@ -328,8 +328,18 @@ connect_path ( struct patch_record *pr )
 {
     int r = 0;
 
-    char srcport[512]; // This should really be REAL_JACK_PORT_NAME_SIZE, but in the real world not every system and compiler does C99.
-    char dstport[512];
+    while (!client_active)
+    {
+        sleep(1);
+    }
+
+    char *srcport = alloca(REAL_JACK_PORT_NAME_SIZE);
+    char *dstport = alloca(REAL_JACK_PORT_NAME_SIZE);
+
+    if (!srcport || !dstport)
+    {
+        return;
+    }
 
     snprintf( srcport, REAL_JACK_PORT_NAME_SIZE, "%s:%s", pr->src.client, pr->src.port );
     snprintf( dstport, REAL_JACK_PORT_NAME_SIZE, "%s:%s", pr->dst.client, pr->dst.port );

[falkTX]

if pipewire crashes and real jack does not, we should fix pipewire side instead of trying to mitigate the issues.

where does the overflow happen?

[ventosus]

jackpatch crashes, it's obviously the clients fault, the line number is in the GDB log.

connect_path on pipewire-jack is obviously called before REAL_JACK_PORT_NAME_SIZE is set, so snprintf tries to write into a buffer of size 0/unknown.

jack_port_name_size should be called before jack_activate, this fixes the issue for me.

diff --git a/src/jackpatch.c b/src/jackpatch.c
index bc9303b..20a826e 100644
--- a/src/jackpatch.c
+++ b/src/jackpatch.c
@@ -61,7 +61,7 @@ int nsm_is_active;
 
 char *project_file;
 
-int REAL_JACK_PORT_NAME_SIZE; //defined after jack client activated
+int REAL_JACK_PORT_NAME_SIZE = 0; //defined after jack client activated
 
 #undef VERSION
 #define APP_TITLE "JACKPatch"
@@ -328,8 +328,18 @@ connect_path ( struct patch_record *pr )
 {
     int r = 0;
 
-    char srcport[512]; // This should really be REAL_JACK_PORT_NAME_SIZE, but in the real world not every system and compiler does C99.
-    char dstport[512];
+    if (REAL_JACK_PORT_NAME_SIZE == 0)
+    {
+        return;
+    }
+
+    char *srcport = alloca(REAL_JACK_PORT_NAME_SIZE);
+    char *dstport = alloca(REAL_JACK_PORT_NAME_SIZE);
+
+    if (!srcport || !dstport)
+    {
+        return;
+    }
 
     snprintf( srcport, REAL_JACK_PORT_NAME_SIZE, "%s:%s", pr->src.client, pr->src.port );
     snprintf( dstport, REAL_JACK_PORT_NAME_SIZE, "%s:%s", pr->dst.client, pr->dst.port );
@@ -733,9 +743,9 @@ maybe_activate_jack_client ( void )
 {
     if ( ! client_active )
     {
+        REAL_JACK_PORT_NAME_SIZE = jack_port_name_size(); //global. This is client+port+1. 64 + 256 + 1 = 321 on Linux.
         jack_activate( client );
         client_active = 1;
-        REAL_JACK_PORT_NAME_SIZE = jack_port_name_size(); //global. This is client+port+1. 64 + 256 + 1 = 321 on Linux.
     }
 }

[falkTX]

ok that part makes sense. but there is no need to change the stack array into alloca, that is an unrelated change.
I have seen issues before due to the use of alloca, as it is not very portable. even its own documentation recommends to not use it. the old code with a fixed 512 array didnt cause any harm there.

[ventosus]

The changes are related, they both concern 'REAL_JACK_PORT_NAME_SIZE'. If a fixed buffer size of 512 bytes is ok (which I don't think it is), then there is no need for 'REAL_JACK_PORT_NAME_SIZE' in the first place and you can just use 'snprintf(buf, sizeof(buf), ...)'.

If 'alloca' is not portable, just use an 'malloc'.