jack_lsp: An attempt to use --server/-s argument fails with buffer overflow

[unclechu]

$ jack_lsp -s default
*** buffer overflow detected ***: terminated
Aborted (core dumped)

Version 4 (GIt tag) of this repo.

[unclechu]

I think I figured it out, there’s no string termination in the copied optarg. Here is some debug code I used to test it:

			for (int i=0; i<strlen(optarg); ++i)
				printf("str %d: “%c”\n", i, optarg[i]);
			printf("done\n");

str 0: “d”
str 1: “e”
str 2: “f”
str 3: “a”
str 4: “u”
str 5: “l”
str 6: “t”
done

So, there are only chars from the argument in there, no string termination char. Here is a patch that fixes the bug:

diff --git a/tools/lsp.c b/tools/lsp.c
index 50868b3..f043ad2 100644
--- a/tools/lsp.c
+++ b/tools/lsp.c
@@ -122,7 +122,8 @@ main (int argc, char *argv[])
 	while ((c = getopt_long (argc, argv, "s:AclLphvtuU", long_options, &option_index)) >= 0) {
 		switch (c) {
 		case 's':
-			server_name = (char *) malloc (sizeof (char) * strlen(optarg));
+			server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
+			server_name[strlen(optarg)] = '\0';
 			strcpy (server_name, optarg);
 			options |= JackServerName;
 			break;

[unclechu]

Created a merge request with the fix: #89

[unclechu]

In case any Nix folk in need stumble upon this before it is fixed here is what you can use as a temporary solution:

(pkgs.jack-example-tools.overrideAttrs (old: {
  patches = (old.patches or []) ++ [
    # Big fix for “jack_lsp” “--server” argument buffer overflow.
    # Track the bug fixing progress here:
    # https://github.com/jackaudio/jack-example-tools/issues/88
    (pkgs.fetchpatch {
      name = "jack_lsp-fix-jack-server-argument-buffer-overflow.patch";
      url = "https://github.com/jackaudio/jack-example-tools/pull/89/commits/62aeea4c432c8f91b14888c4dc4c310ef762a865.patch";
      hash = "sha256-TbgJdwsxo9K6wTQ46yHLYDbIJkINNARlb332qC8TWlM=";
    })
  ];
}))

[unclechu]

I grepped other argument parsing pieces for other apps from the repo:

$ git grep optarg | rg malloc | rg -v lsp
example-clients/metro.c:                        bpm_string = (char *) malloc ((strlen (optarg) + 5) * sizeof (char));
example-clients/metro.c:                        client_name = (char *) malloc ((strlen (optarg) + 1) * sizeof (char));
tools/connect.c:                        server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
tools/netsource.c:                peer_ip = (char *) malloc (sizeof (char) * strlen (optarg) + 1);
tools/netsource.c:                client_name = (char *) malloc (sizeof (char) * strlen (optarg) + 1);
tools/netsource.c:                server_name = (char *) malloc (sizeof (char) * strlen (optarg) + 1);
tools/wait.c:                   server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
tools/wait.c:                   client_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));

And all of them except lsp.c seem to add one extra char to the total count. Though for instance in wait.c the string termination char is not set explicitly:

		case 's':
			server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
			strcpy (server_name, optarg);
			options |= JackServerName;
			break;

Is this okay? Maybe I’m wrong about optarg not having the termination char, it could be just that strlen(optarg) does not take that termination char into account, which is how it is supposed to be, duh. So the second server_name[strlen(optarg)] = '\0'; line in the fix is probably a redundant one.