Spaces in GPT partition names not properly escaped and general question on escaping for generated scripts

[cscutcher]

Noticed during some experimentation that spaces in GPT partition names cause problems. Irritating I've lost the exact error output but in the case of a GPT partition name Basic Data Partition, parted complains that Data isn't a valid fs-type i.e. parted interprets the partition name as Basic and then treats Data and Partition as the subsequent arguments. Even escaping the name with strings.escapeShellArg isn't sufficient.

I note that throughout the code, there appears to be little implicit escaping of strings (via strings.escapeShellArg or similar) which might be an intentional decision to keep things explicit, or more likely just something that's not done yet?

In either case, arguments to parted mkpart are a bit more awkward since even if the user thinks to wrap name in escapeShellArg this isn't sufficient, since bash strips the quotes before the command is interpreted by parted. For example this stackoverflow shows an example of the required quoting;

$ sudo parted /dev/sdb mkpart '"Name of the partition"' 0% 100%

I'm trying to work around this on the config side by wrapping the name arguments with this;

escape_parted_name = inp: l.strings.escapeShellArg "\"${inp}\"";

Which I was tempted to submit as a fix targeted specifically at the partition script generation

https://github.com/nix-community/disko/blob/d7e178126f05d9b1e5bfbf115c36ec9f75b85e8b/types.nix#L770

But then it occurred to me that would introduce an inconsistency, with this one argument being implicitly escaped but the majority of other arguments to parted, and in other generated scripts (names, options etc), being left raw.

Any thoughts to how you guys would like to see this handled? Any existing plans to clean up some of the code around script generation and argument handling?

[Mic92]

Maybe all those commands should just use lib.toGNUCommandLineShell for proper escaping.

[cscutcher]

Oh man, how have I never come across toGNUCommandLineShell before! Nice!

Maybe all those commands should just use lib.toGNUCommandLineShell for proper escaping.

Yeah, I think that's a good call, given the amount of script generation going on. Although I think it'd still need special handling for the case of parted to ensure that quotes remain when parted parses the inner command (i.e. they're not all stripped by bash).

[cscutcher]

I'm up for having a swing at this myself, but might be a while since not much free time to code at the moment.

[Lassulus]

not sure if thats still a problem? maybe PRing an example which causes the break is enough so I can fix it

[cscutcher]

Sorry, have been more busy than I expected this year!

As far as I can tell, it's still an issue as of 4eed2457b053c4bbad7d90d2b3a1d539c2c9009c . So generating diskoScript will result with something like;

parted -s /dev/disk/by-id/nvme-Samsung_SSD_970_PRO_1TB_SXXXXXXXXXXXXX -- mkpart Basic Data Partition ntfs 1049kB 473MB

when I believe the correct escaping (which I achieve with the escape_parted_name above) should be;

parted -s /dev/disk/by-id/nvme-Samsung_SSD_970_PRO_1TB_SXXXXXXXXXXXXX -- mkpart '"Basic Data Partition"' ntfs 1049kB 473MB

(slightly modified output to vaguely anonymise drive info)

Extracting from my config I think this should be enough to reproduce;

    disko.devices = {
      disk.root = {
        type = "disk";
        device = "/dev/sda1";
        content = {
          type = "table";
          format = "gpt";
          partitions = [
            {
              start = "1049kB";
              end = "473MB";
              name = "Basic Data Partition";
              fs-type = "ntfs";
              flags = [
                "hidden"
                "diag"
              ];
              content = {
                type = "filesystem";
                format = "ntfs";
                mountpoint = "/mnt/ignore/windows_recovery";
              };
            }
          ];
        };
      };
    };

will look at making an example PR when I can.

[cscutcher]

Had a quick bash at reproducing with examples/test. I hadn't realised there was a new syntax for partition tables, but it seems to fail just the same (see PR).

Something that occurred to me while testing is that escaping the /dev/disk/by-partlabel isn't as simple as quoting, since it does the \x20 escaping thing (not sure how best to refer to it!);

$ ls /dev/disk/by-partlabel/Basic* 
'/dev/disk/by-partlabel/Basic\x20data\x20partition'

You might already have solved this problem elsewhere, but just in case it's helpful, I note that the systemd-escape command will escape spaces in the same way;

$ systemd-escape "Basic Data Partition"
Basic\x20Data\x20Partition