[PATCH] ld: Add --encoded-package-metadata

[Jan Beulich]

On 15.07.2024 20:43, Benjamin Drung wrote:
> Specifying the compiler flag `-Wl,--package-metadata=<JSON>` might not
> work, because the shells might eat the quotation marks and the compiler
> splits the JSON at the commas.
> 
> Ubuntu tried to using a specs file to set `--package-metadata` but that
> turned out to be too fragile. autopkgtests might use the compiler flags
> but the needed environment variables are not set in the test
> environment. Debugging a crash of an application build with the -spec
> parameter lacks the environment variables. People like to iteratively
> continue building the software in the build directory while hacking on
> the package and then have no environment variable set.
> 
> So introduce a `--encoded-package-metadata` linker flag that takes a
> percent-encoded JSON. Percent-encoding is used because it is a
> standard and simple to implement.
> 
> Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>

Fundamentally fine, but please add a testcase (even if it's a contrived one).
And there are a number of style issues (see below).

> --- a/ld/emultempl/elf.em
> +++ b/ld/emultempl/elf.em
> @@ -813,6 +813,7 @@ EOF
>  fi
>  fragment <<EOF
>      {"build-id", optional_argument, NULL, OPTION_BUILD_ID},
> +    {"encoded-package-metadata", optional_argument, NULL, OPTION_ENCODED_PACKAGE_METADATA},
>      {"package-metadata", optional_argument, NULL, OPTION_PACKAGE_METADATA},
>      {"compress-debug-sections", required_argument, NULL, OPTION_COMPRESS_DEBUG},
>      {"rosegment", no_argument, NULL, OPTION_ROSEGMENT},
> @@ -864,6 +865,23 @@ gld${EMULATION_NAME}_handle_option (int optc)
>  	ldelf_emit_note_gnu_build_id = xstrdup (optarg);
>        break;
>  
> +    case OPTION_ENCODED_PACKAGE_METADATA:
> +      free ((char *) ldelf_emit_note_fdo_package_metadata);

This is, I think, the common GNU way of writing casts. I wouldn't insist on
the blank after the closing paren here if it was missing, yet further down
it is. My minimal request is: Please be consistent within a patch.

> +      ldelf_emit_note_fdo_package_metadata = NULL;
> +      if (optarg != NULL) {

Opening brace placement is wrong thoughout the entire patch.

> +	size_t len = strlen(optarg);

Blank missing ahead of the opening paren (also elsewhere).

> +	if (len > 0) {
> +	  ldelf_emit_note_fdo_package_metadata = xmalloc (len + 1);
> +	  int len = percent_decode (optarg,
> +	                           ldelf_emit_note_fdo_package_metadata);
> +	  if (len < 0) {
> +	    einfo (_ ("%F%P: Failed to decode percent-encoded package metadata"
> +	          " \`%s'\n"), optarg);

Yes, there are three examples of "_ (" in ld sources. The canonical spelling
is "_(" though, unlike function calls.

> --- a/ld/ldmisc.c
> +++ b/ld/ldmisc.c
> @@ -770,3 +770,53 @@ ld_abort (const char *file, int line, const char *fn)
>    einfo (_("%F%P: please report this bug\n"));
>    xexit (1);
>  }
> +
> +/* Decode a hexadecimal character. Return -1 on error. */
> +static int
> +hexdecode(char c) {
> +  if ('0' <= c && c <= '9') {
> +    return c - '0';
> +  }
> +  if ('A' <= c && c <= 'F') {
> +    return c - 'A' + 10;
> +  }
> +  if ('a' <= c && c <= 'f') {
> +    return c - 'a' + 10;
> +  }
> +  return -1;
> +}
> +
> +/* Decode a percent-encoded string. dst must be at least the same size as src.
> +   It can be converted in place. Returns the lenght of the decoded string
> +   (without training null character) or -1 on error. */
> +int
> +percent_decode(char *src, char *dst) {

"const char *" for "src" please.

> +  int length = 0;
> +  while (*src != '\0') {
> +    char c = *src++;
> +    if (c != '%') {
> +      *dst++ = c;
> +      length += 1;
> +      continue;
> +    }
> +    char next1 = *src++;
> +    if (next1 == '%') {
> +      // Encoded %
> +      *dst++ = c;
> +      length += 1;
> +      continue;
> +    }
> +    int hex1 = hexdecode(next1);
> +    if (hex1 == -1) {
> +      return -1;
> +    }
> +    int hex2 = hexdecode(*src++);
> +    if (hex2 == -1) {
> +      return -1;
> +    }
> +    *dst++ = ((char)hex1 << 4) + (char)hex2;

I'm curious: What use are the casts here? Especially the left one does,
afaict, absolutely nothing: hex1 is converted to char (without changing the
value, given what hexdecode() can return) just to then be promoted back to
int.

Jan