jackpatch crashing at load (buffer overflow)

[ventosus]

jackpatch crashing at load (buffer overflow)

[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'.