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

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

[Benjamin Drung]

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>
---
 ld/NEWS             |  5 +++++
 ld/emultempl/elf.em | 18 ++++++++++++++++
 ld/ld.texi          | 15 +++++++++++++-
 ld/ldlex.h          |  1 +
 ld/ldmisc.c         | 50 +++++++++++++++++++++++++++++++++++++++++++++
 ld/ldmisc.h         |  1 +
 ld/lexsup.c         |  2 ++
 7 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/ld/NEWS b/ld/NEWS
index e0b9341a8ef..250ce353068 100644
--- a/ld/NEWS
+++ b/ld/NEWS
@@ -12,6 +12,11 @@
 
 * Add -plugin-save-temps to store plugin intermediate files permanently.
 
+* The ELF linker now supports a new --encoded-package-metadata option that
+  is equivalent to --package-metadata, but takes the JSON payload
+  percent-encoded to ease passing around this option without getting the
+  JSON payload corrupted.
+
 Changes in 2.42:
 
 * Add -z mark-plt/-z nomark-plt options to x86-64 ELF linker to mark PLT
diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
index 863657e12f5..a6cbf380508 100644
--- 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);
+      ldelf_emit_note_fdo_package_metadata = NULL;
+      if (optarg != NULL) {
+	size_t len = strlen(optarg);
+	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);
+	  }
+	}
+      }
+      break;
+
     case OPTION_PACKAGE_METADATA:
       free ((char *) ldelf_emit_note_fdo_package_metadata);
       ldelf_emit_note_fdo_package_metadata = NULL;
diff --git a/ld/ld.texi b/ld/ld.texi
index 89e3913317a..28733b1ae8e 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -3234,6 +3234,19 @@ string identifying the original linked file does not change.
 Passing @code{none} for @var{style} disables the setting from any
 @code{--build-id} options earlier on the command line.
 
+@kindex --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+@item --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+Request the creation of a @code{.note.package} ELF note section.  The
+contents of the note are in JSON format, as per the package metadata
+specification.  For more information see:
+https://systemd.io/ELF_PACKAGE_METADATA/
+If the PERCENT_ENCODED_JSON argument is missing/empty then this will
+disable the creation of the metadata note, if one had been enabled by
+an earlier occurrence of the --encoded-package-metadata or
+--package-metadata option. PERCENT_ENCODED_JSON will be percent-decoded.
+If the linker has been built with libjansson, then the JSON string
+will be validated.
+
 @kindex --package-metadata=@var{JSON}
 @item --package-metadata=@var{JSON}
 Request the creation of a @code{.note.package} ELF note section.  The
@@ -3242,7 +3255,7 @@ specification.  For more information see:
 https://systemd.io/ELF_PACKAGE_METADATA/
 If the JSON argument is missing/empty then this will disable the
 creation of the metadata note, if one had been enabled by an earlier
-occurrence of the --package-metadata option.
+occurrence of the --encoded-package-metadata or --package-metadata option.
 If the linker has been built with libjansson, then the JSON string
 will be validated.
 @end table
diff --git a/ld/ldlex.h b/ld/ldlex.h
index defe3fcbbb9..225d95b1e90 100644
--- a/ld/ldlex.h
+++ b/ld/ldlex.h
@@ -284,6 +284,7 @@ enum option_values
   OPTION_EH_FRAME_HDR,
   OPTION_NO_EH_FRAME_HDR,
   OPTION_HASH_STYLE,
+  OPTION_ENCODED_PACKAGE_METADATA,
   OPTION_PACKAGE_METADATA,
   OPTION_AUDIT,
   OPTION_COMPRESS_DEBUG,
diff --git a/ld/ldmisc.c b/ld/ldmisc.c
index 180b24b3448..960a5d3742f 100644
--- 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) {
+  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;
+    length += 1;
+  }
+  *dst = '\0';
+  return length;
+}
diff --git a/ld/ldmisc.h b/ld/ldmisc.h
index 20289127c0a..76a2d72125d 100644
--- a/ld/ldmisc.h
+++ b/ld/ldmisc.h
@@ -39,5 +39,6 @@ do { info_assert(__FILE__,__LINE__); } while (0)
 extern void print_spaces (int);
 #define print_space() print_spaces (1)
 extern void print_nl (void);
+extern int percent_decode(char *, char *);
 
 #endif
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 4aa0124ce2f..60e329a713f 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -2279,6 +2279,8 @@ elf_static_list_options (FILE *file)
   fprintf (file, _("\
   --build-id[=STYLE]          Generate build ID note\n"));
   fprintf (file, _("\
+  --encoded-package-metadata[=PERCENT_ENCODED_JSON]   Generate package metadata note\n"));
+  fprintf (file, _("\
   --package-metadata[=JSON]   Generate package metadata note\n"));
   fprintf (file, _("\
   --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi|zstd]\n\
-- 
2.43.0

Re: [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

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

[Benjamin Drung]

On Tue, 2024-07-16 at 08:19 +0200, Jan Beulich wrote:
> 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).
> 

I am working on a test case, but I'll send the reformatted code as patch
v2 for allowing to review the formatting changes. While writing the
code, I tried to follow what the existing code did, but failed horribly.

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

That code line is used identical some lines below.

> > +      ldelf_emit_note_fdo_package_metadata = NULL;
> > +      if (optarg != NULL) {
> 
> Opening brace placement is wrong thoughout the entire patch.
> 

Hopefully fixed everywhere.

> > +	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.
> 

Changed.

> > +  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.

I changed it to do the char conversion after the full calculation.

-- 
Benjamin Drung
Debian & Ubuntu Developer

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

[Benjamin Drung]

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>
---
 ld/NEWS             |  5 +++++
 ld/emultempl/elf.em | 19 +++++++++++++++++
 ld/ld.texi          | 15 +++++++++++++-
 ld/ldlex.h          |  1 +
 ld/ldmisc.c         | 50 +++++++++++++++++++++++++++++++++++++++++++++
 ld/ldmisc.h         |  1 +
 ld/lexsup.c         |  2 ++
 7 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/ld/NEWS b/ld/NEWS
index e0b9341a8ef..250ce353068 100644
--- a/ld/NEWS
+++ b/ld/NEWS
@@ -12,6 +12,11 @@
 
 * Add -plugin-save-temps to store plugin intermediate files permanently.
 
+* The ELF linker now supports a new --encoded-package-metadata option that
+  is equivalent to --package-metadata, but takes the JSON payload
+  percent-encoded to ease passing around this option without getting the
+  JSON payload corrupted.
+
 Changes in 2.42:
 
 * Add -z mark-plt/-z nomark-plt options to x86-64 ELF linker to mark PLT
diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
index 863657e12f5..2ae6ea2bcc8 100644
--- 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,24 @@ 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);
+      ldelf_emit_note_fdo_package_metadata = NULL;
+      if (optarg != NULL)
+	{
+	  size_t len = strlen (optarg);
+	  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);
+	    }
+        }
+      break;
+
     case OPTION_PACKAGE_METADATA:
       free ((char *) ldelf_emit_note_fdo_package_metadata);
       ldelf_emit_note_fdo_package_metadata = NULL;
diff --git a/ld/ld.texi b/ld/ld.texi
index 89e3913317a..28733b1ae8e 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -3234,6 +3234,19 @@ string identifying the original linked file does not change.
 Passing @code{none} for @var{style} disables the setting from any
 @code{--build-id} options earlier on the command line.
 
+@kindex --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+@item --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+Request the creation of a @code{.note.package} ELF note section.  The
+contents of the note are in JSON format, as per the package metadata
+specification.  For more information see:
+https://systemd.io/ELF_PACKAGE_METADATA/
+If the PERCENT_ENCODED_JSON argument is missing/empty then this will
+disable the creation of the metadata note, if one had been enabled by
+an earlier occurrence of the --encoded-package-metadata or
+--package-metadata option. PERCENT_ENCODED_JSON will be percent-decoded.
+If the linker has been built with libjansson, then the JSON string
+will be validated.
+
 @kindex --package-metadata=@var{JSON}
 @item --package-metadata=@var{JSON}
 Request the creation of a @code{.note.package} ELF note section.  The
@@ -3242,7 +3255,7 @@ specification.  For more information see:
 https://systemd.io/ELF_PACKAGE_METADATA/
 If the JSON argument is missing/empty then this will disable the
 creation of the metadata note, if one had been enabled by an earlier
-occurrence of the --package-metadata option.
+occurrence of the --encoded-package-metadata or --package-metadata option.
 If the linker has been built with libjansson, then the JSON string
 will be validated.
 @end table
diff --git a/ld/ldlex.h b/ld/ldlex.h
index defe3fcbbb9..225d95b1e90 100644
--- a/ld/ldlex.h
+++ b/ld/ldlex.h
@@ -284,6 +284,7 @@ enum option_values
   OPTION_EH_FRAME_HDR,
   OPTION_NO_EH_FRAME_HDR,
   OPTION_HASH_STYLE,
+  OPTION_ENCODED_PACKAGE_METADATA,
   OPTION_PACKAGE_METADATA,
   OPTION_AUDIT,
   OPTION_COMPRESS_DEBUG,
diff --git a/ld/ldmisc.c b/ld/ldmisc.c
index 180b24b3448..b8e4c87740b 100644
--- 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 (const char *src, char *dst)
+{
+  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) + hex2);
+      length += 1;
+    }
+  *dst = '\0';
+  return length;
+}
diff --git a/ld/ldmisc.h b/ld/ldmisc.h
index 20289127c0a..815e8ccb548 100644
--- a/ld/ldmisc.h
+++ b/ld/ldmisc.h
@@ -39,5 +39,6 @@ do { info_assert(__FILE__,__LINE__); } while (0)
 extern void print_spaces (int);
 #define print_space() print_spaces (1)
 extern void print_nl (void);
+extern int percent_decode (const char *, char *);
 
 #endif
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 4aa0124ce2f..60e329a713f 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -2279,6 +2279,8 @@ elf_static_list_options (FILE *file)
   fprintf (file, _("\
   --build-id[=STYLE]          Generate build ID note\n"));
   fprintf (file, _("\
+  --encoded-package-metadata[=PERCENT_ENCODED_JSON]   Generate package metadata note\n"));
+  fprintf (file, _("\
   --package-metadata[=JSON]   Generate package metadata note\n"));
   fprintf (file, _("\
   --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi|zstd]\n\
-- 
2.43.0

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

[Jan Beulich]

On 16.07.2024 10:32, Benjamin Drung wrote:
> On Tue, 2024-07-16 at 08:19 +0200, Jan Beulich wrote:
>> 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).
> 
> I am working on a test case, but I'll send the reformatted code as patch
> v2 for allowing to review the formatting changes.

That looks much better, yes. Thanks.

Jan

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

[Benjamin Drung]

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>
---
 ld/NEWS                              |  5 +++
 ld/emultempl/elf.em                  | 19 +++++++++++
 ld/ld.texi                           | 15 ++++++++-
 ld/ldlex.h                           |  1 +
 ld/ldmisc.c                          | 50 ++++++++++++++++++++++++++++
 ld/ldmisc.h                          |  1 +
 ld/lexsup.c                          |  2 ++
 ld/testsuite/ld-elf/package-note.exp | 18 ++++++++++
 ld/testsuite/ld-elf/package-note2.rd |  6 ++++
 9 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 ld/testsuite/ld-elf/package-note2.rd

diff --git a/ld/NEWS b/ld/NEWS
index e0b9341a8ef..250ce353068 100644
--- a/ld/NEWS
+++ b/ld/NEWS
@@ -12,6 +12,11 @@
 
 * Add -plugin-save-temps to store plugin intermediate files permanently.
 
+* The ELF linker now supports a new --encoded-package-metadata option that
+  is equivalent to --package-metadata, but takes the JSON payload
+  percent-encoded to ease passing around this option without getting the
+  JSON payload corrupted.
+
 Changes in 2.42:
 
 * Add -z mark-plt/-z nomark-plt options to x86-64 ELF linker to mark PLT
diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
index 863657e12f5..1a893f35416 100644
--- 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,24 @@ 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);
+      ldelf_emit_note_fdo_package_metadata = NULL;
+      if (optarg != NULL)
+	{
+	  size_t optarg_len = strlen (optarg);
+	  if (optarg_len > 0)
+	    {
+	      char *package_metadata = xmalloc (optarg_len + 1);
+	      int len = percent_decode (optarg, package_metadata);
+	      if (len < 0)
+		einfo (_("%F%P: Failed to decode percent-encoded package"
+		         " metadata \`%s'\n"), optarg);
+	      ldelf_emit_note_fdo_package_metadata = package_metadata;
+	    }
+        }
+      break;
+
     case OPTION_PACKAGE_METADATA:
       free ((char *) ldelf_emit_note_fdo_package_metadata);
       ldelf_emit_note_fdo_package_metadata = NULL;
diff --git a/ld/ld.texi b/ld/ld.texi
index 89e3913317a..28733b1ae8e 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -3234,6 +3234,19 @@ string identifying the original linked file does not change.
 Passing @code{none} for @var{style} disables the setting from any
 @code{--build-id} options earlier on the command line.
 
+@kindex --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+@item --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+Request the creation of a @code{.note.package} ELF note section.  The
+contents of the note are in JSON format, as per the package metadata
+specification.  For more information see:
+https://systemd.io/ELF_PACKAGE_METADATA/
+If the PERCENT_ENCODED_JSON argument is missing/empty then this will
+disable the creation of the metadata note, if one had been enabled by
+an earlier occurrence of the --encoded-package-metadata or
+--package-metadata option. PERCENT_ENCODED_JSON will be percent-decoded.
+If the linker has been built with libjansson, then the JSON string
+will be validated.
+
 @kindex --package-metadata=@var{JSON}
 @item --package-metadata=@var{JSON}
 Request the creation of a @code{.note.package} ELF note section.  The
@@ -3242,7 +3255,7 @@ specification.  For more information see:
 https://systemd.io/ELF_PACKAGE_METADATA/
 If the JSON argument is missing/empty then this will disable the
 creation of the metadata note, if one had been enabled by an earlier
-occurrence of the --package-metadata option.
+occurrence of the --encoded-package-metadata or --package-metadata option.
 If the linker has been built with libjansson, then the JSON string
 will be validated.
 @end table
diff --git a/ld/ldlex.h b/ld/ldlex.h
index defe3fcbbb9..225d95b1e90 100644
--- a/ld/ldlex.h
+++ b/ld/ldlex.h
@@ -284,6 +284,7 @@ enum option_values
   OPTION_EH_FRAME_HDR,
   OPTION_NO_EH_FRAME_HDR,
   OPTION_HASH_STYLE,
+  OPTION_ENCODED_PACKAGE_METADATA,
   OPTION_PACKAGE_METADATA,
   OPTION_AUDIT,
   OPTION_COMPRESS_DEBUG,
diff --git a/ld/ldmisc.c b/ld/ldmisc.c
index 180b24b3448..b8e4c87740b 100644
--- 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 (const char *src, char *dst)
+{
+  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) + hex2);
+      length += 1;
+    }
+  *dst = '\0';
+  return length;
+}
diff --git a/ld/ldmisc.h b/ld/ldmisc.h
index 20289127c0a..815e8ccb548 100644
--- a/ld/ldmisc.h
+++ b/ld/ldmisc.h
@@ -39,5 +39,6 @@ do { info_assert(__FILE__,__LINE__); } while (0)
 extern void print_spaces (int);
 #define print_space() print_spaces (1)
 extern void print_nl (void);
+extern int percent_decode (const char *, char *);
 
 #endif
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 4aa0124ce2f..60e329a713f 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -2279,6 +2279,8 @@ elf_static_list_options (FILE *file)
   fprintf (file, _("\
   --build-id[=STYLE]          Generate build ID note\n"));
   fprintf (file, _("\
+  --encoded-package-metadata[=PERCENT_ENCODED_JSON]   Generate package metadata note\n"));
+  fprintf (file, _("\
   --package-metadata[=JSON]   Generate package metadata note\n"));
   fprintf (file, _("\
   --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi|zstd]\n\
diff --git a/ld/testsuite/ld-elf/package-note.exp b/ld/testsuite/ld-elf/package-note.exp
index 9f18705bd65..b4feef541b4 100644
--- a/ld/testsuite/ld-elf/package-note.exp
+++ b/ld/testsuite/ld-elf/package-note.exp
@@ -42,4 +42,22 @@ run_ld_link_tests [list \
 	{{readelf {--notes} package-note.rd}} \
 	"package-note.o" \
     ] \
+    [list \
+	"package-note1b.o" \
+	"--encoded-package-metadata=%7B%22foo%22%3A%22bar%22%7D" \
+	"" \
+	"" \
+	{start.s} \
+	{{readelf {--notes} package-note.rd}} \
+	"package-note1b.o" \
+    ] \
+    [list \
+	"package-note2.o" \
+	"--encoded-package-metadata=%7B%22name%22:%22binutils%22%2C%22foo%22%3A%22bar%22%7d" \
+	"" \
+	"" \
+	{start.s} \
+	{{readelf {--notes} package-note2.rd}} \
+	"package-note2.o" \
+    ] \
 ]
diff --git a/ld/testsuite/ld-elf/package-note2.rd b/ld/testsuite/ld-elf/package-note2.rd
new file mode 100644
index 00000000000..c24c37825ad
--- /dev/null
+++ b/ld/testsuite/ld-elf/package-note2.rd
@@ -0,0 +1,6 @@
+#...
+Displaying notes found in: \.note\.package
+\s+Owner\s+Data\s+size\s+Description
+\s+FDO\s+0x00000020\s+(Unknown note type:\s+\(0xcafe1a7e\)|FDO_PACKAGING_METADATA)
+\s+(description data:\s+.*|Packaging Metadata:\s+{"name":"binutils","foo":"bar"})
+#pass
-- 
2.43.0

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

[Matthias Klose]

On 16.07.24 10:57, Jan Beulich wrote:
> On 16.07.2024 10:32, Benjamin Drung wrote:
>> On Tue, 2024-07-16 at 08:19 +0200, Jan Beulich wrote:
>>> 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).
>>
>> I am working on a test case, but I'll send the reformatted code as patch
>> v2 for allowing to review the formatting changes.
> 
> That looks much better, yes. Thanks.

is that an approval, and can be committed?

Matthias

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

[Benjamin Drung]

On Thu, 2024-07-18 at 10:17 +0200, Matthias Klose wrote:
> On 16.07.24 10:57, Jan Beulich wrote:
> > On 16.07.2024 10:32, Benjamin Drung wrote:
> > > On Tue, 2024-07-16 at 08:19 +0200, Jan Beulich wrote:
> > > > 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).
> > > 
> > > I am working on a test case, but I'll send the reformatted code as patch
> > > v2 for allowing to review the formatting changes.
> > 
> > That looks much better, yes. Thanks.
> 
> is that an approval, and can be committed?

FYI: Adding --encoded-package-metadata was merged in mold:
https://github.com/rui314/mold/pull/1308

-- 
Benjamin Drung
Debian & Ubuntu Developer

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

[Jan Beulich]

On 18.07.2024 10:17, Matthias Klose wrote:
> On 16.07.24 10:57, Jan Beulich wrote:
>> On 16.07.2024 10:32, Benjamin Drung wrote:
>>> On Tue, 2024-07-16 at 08:19 +0200, Jan Beulich wrote:
>>>> 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).
>>>
>>> I am working on a test case, but I'll send the reformatted code as patch
>>> v2 for allowing to review the formatting changes.
>>
>> That looks much better, yes. Thanks.
> 
> is that an approval, and can be committed?

There was a v3 submitted, which still needs looking at. I can't (won't)
approve the testcase addition that was still missing in v2 without
having looked at that.

Jan

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

[Benjamin Drung]

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>
---
 ld/NEWS                              |  5 +++
 ld/emultempl/elf.em                  | 19 +++++++++++
 ld/ld.texi                           | 15 ++++++++-
 ld/ldlex.h                           |  1 +
 ld/ldmisc.c                          | 49 ++++++++++++++++++++++++++++
 ld/ldmisc.h                          |  1 +
 ld/lexsup.c                          |  2 ++
 ld/testsuite/ld-elf/package-note.exp | 18 ++++++++++
 ld/testsuite/ld-elf/package-note2.rd |  6 ++++
 9 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 ld/testsuite/ld-elf/package-note2.rd

diff --git a/ld/NEWS b/ld/NEWS
index e0b9341a8ef..250ce353068 100644
--- a/ld/NEWS
+++ b/ld/NEWS
@@ -12,6 +12,11 @@
 
 * Add -plugin-save-temps to store plugin intermediate files permanently.
 
+* The ELF linker now supports a new --encoded-package-metadata option that
+  is equivalent to --package-metadata, but takes the JSON payload
+  percent-encoded to ease passing around this option without getting the
+  JSON payload corrupted.
+
 Changes in 2.42:
 
 * Add -z mark-plt/-z nomark-plt options to x86-64 ELF linker to mark PLT
diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
index 863657e12f5..1a893f35416 100644
--- 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,24 @@ 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);
+      ldelf_emit_note_fdo_package_metadata = NULL;
+      if (optarg != NULL)
+	{
+	  size_t optarg_len = strlen (optarg);
+	  if (optarg_len > 0)
+	    {
+	      char *package_metadata = xmalloc (optarg_len + 1);
+	      int len = percent_decode (optarg, package_metadata);
+	      if (len < 0)
+		einfo (_("%F%P: Failed to decode percent-encoded package"
+		         " metadata \`%s'\n"), optarg);
+	      ldelf_emit_note_fdo_package_metadata = package_metadata;
+	    }
+        }
+      break;
+
     case OPTION_PACKAGE_METADATA:
       free ((char *) ldelf_emit_note_fdo_package_metadata);
       ldelf_emit_note_fdo_package_metadata = NULL;
diff --git a/ld/ld.texi b/ld/ld.texi
index 89e3913317a..28733b1ae8e 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -3234,6 +3234,19 @@ string identifying the original linked file does not change.
 Passing @code{none} for @var{style} disables the setting from any
 @code{--build-id} options earlier on the command line.
 
+@kindex --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+@item --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+Request the creation of a @code{.note.package} ELF note section.  The
+contents of the note are in JSON format, as per the package metadata
+specification.  For more information see:
+https://systemd.io/ELF_PACKAGE_METADATA/
+If the PERCENT_ENCODED_JSON argument is missing/empty then this will
+disable the creation of the metadata note, if one had been enabled by
+an earlier occurrence of the --encoded-package-metadata or
+--package-metadata option. PERCENT_ENCODED_JSON will be percent-decoded.
+If the linker has been built with libjansson, then the JSON string
+will be validated.
+
 @kindex --package-metadata=@var{JSON}
 @item --package-metadata=@var{JSON}
 Request the creation of a @code{.note.package} ELF note section.  The
@@ -3242,7 +3255,7 @@ specification.  For more information see:
 https://systemd.io/ELF_PACKAGE_METADATA/
 If the JSON argument is missing/empty then this will disable the
 creation of the metadata note, if one had been enabled by an earlier
-occurrence of the --package-metadata option.
+occurrence of the --encoded-package-metadata or --package-metadata option.
 If the linker has been built with libjansson, then the JSON string
 will be validated.
 @end table
diff --git a/ld/ldlex.h b/ld/ldlex.h
index defe3fcbbb9..225d95b1e90 100644
--- a/ld/ldlex.h
+++ b/ld/ldlex.h
@@ -284,6 +284,7 @@ enum option_values
   OPTION_EH_FRAME_HDR,
   OPTION_NO_EH_FRAME_HDR,
   OPTION_HASH_STYLE,
+  OPTION_ENCODED_PACKAGE_METADATA,
   OPTION_PACKAGE_METADATA,
   OPTION_AUDIT,
   OPTION_COMPRESS_DEBUG,
diff --git a/ld/ldmisc.c b/ld/ldmisc.c
index 180b24b3448..ff98226afd6 100644
--- a/ld/ldmisc.c
+++ b/ld/ldmisc.c
@@ -770,3 +770,52 @@ 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 (const char *src, char *dst)
+{
+  int length = 0;
+  while (*src != '\0')
+    {
+      char c = *src++;
+      if (c != '%')
+	{
+	  *dst++ = c;
+	  length += 1;
+	  continue;
+	}
+      char next1 = *src++;
+      if (next1 == '%')
+	{
+	  *dst++ = '%';
+	  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) + hex2);
+      length += 1;
+    }
+  *dst = '\0';
+  return length;
+}
diff --git a/ld/ldmisc.h b/ld/ldmisc.h
index 20289127c0a..815e8ccb548 100644
--- a/ld/ldmisc.h
+++ b/ld/ldmisc.h
@@ -39,5 +39,6 @@ do { info_assert(__FILE__,__LINE__); } while (0)
 extern void print_spaces (int);
 #define print_space() print_spaces (1)
 extern void print_nl (void);
+extern int percent_decode (const char *, char *);
 
 #endif
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 4aa0124ce2f..60e329a713f 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -2279,6 +2279,8 @@ elf_static_list_options (FILE *file)
   fprintf (file, _("\
   --build-id[=STYLE]          Generate build ID note\n"));
   fprintf (file, _("\
+  --encoded-package-metadata[=PERCENT_ENCODED_JSON]   Generate package metadata note\n"));
+  fprintf (file, _("\
   --package-metadata[=JSON]   Generate package metadata note\n"));
   fprintf (file, _("\
   --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi|zstd]\n\
diff --git a/ld/testsuite/ld-elf/package-note.exp b/ld/testsuite/ld-elf/package-note.exp
index 9f18705bd65..b4feef541b4 100644
--- a/ld/testsuite/ld-elf/package-note.exp
+++ b/ld/testsuite/ld-elf/package-note.exp
@@ -42,4 +42,22 @@ run_ld_link_tests [list \
 	{{readelf {--notes} package-note.rd}} \
 	"package-note.o" \
     ] \
+    [list \
+	"package-note1b.o" \
+	"--encoded-package-metadata=%7B%22foo%22%3A%22bar%22%7D" \
+	"" \
+	"" \
+	{start.s} \
+	{{readelf {--notes} package-note.rd}} \
+	"package-note1b.o" \
+    ] \
+    [list \
+	"package-note2.o" \
+	"--encoded-package-metadata=%7B%22name%22:%22binutils%22%2C%22foo%22%3A%22bar%22%7d" \
+	"" \
+	"" \
+	{start.s} \
+	{{readelf {--notes} package-note2.rd}} \
+	"package-note2.o" \
+    ] \
 ]
diff --git a/ld/testsuite/ld-elf/package-note2.rd b/ld/testsuite/ld-elf/package-note2.rd
new file mode 100644
index 00000000000..c24c37825ad
--- /dev/null
+++ b/ld/testsuite/ld-elf/package-note2.rd
@@ -0,0 +1,6 @@
+#...
+Displaying notes found in: \.note\.package
+\s+Owner\s+Data\s+size\s+Description
+\s+FDO\s+0x00000020\s+(Unknown note type:\s+\(0xcafe1a7e\)|FDO_PACKAGING_METADATA)
+\s+(description data:\s+.*|Packaging Metadata:\s+{"name":"binutils","foo":"bar"})
+#pass
-- 
2.43.0

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 10154 bytes --]

On Thu, Jul 18, 2024, 5:05 PM Benjamin Drung <benjamin.drung@canonical.com>
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.
>

Please open a bug report for this feature request.
A test case is needed to show why this feature is needed.

Thanks.


> Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
> ---
>  ld/NEWS                              |  5 +++
>  ld/emultempl/elf.em                  | 19 +++++++++++
>  ld/ld.texi                           | 15 ++++++++-
>  ld/ldlex.h                           |  1 +
>  ld/ldmisc.c                          | 49 ++++++++++++++++++++++++++++
>  ld/ldmisc.h                          |  1 +
>  ld/lexsup.c                          |  2 ++
>  ld/testsuite/ld-elf/package-note.exp | 18 ++++++++++
>  ld/testsuite/ld-elf/package-note2.rd |  6 ++++
>  9 files changed, 115 insertions(+), 1 deletion(-)
>  create mode 100644 ld/testsuite/ld-elf/package-note2.rd
>
> diff --git a/ld/NEWS b/ld/NEWS
> index e0b9341a8ef..250ce353068 100644
> --- a/ld/NEWS
> +++ b/ld/NEWS
> @@ -12,6 +12,11 @@
>
>  * Add -plugin-save-temps to store plugin intermediate files permanently.
>
> +* The ELF linker now supports a new --encoded-package-metadata option that
> +  is equivalent to --package-metadata, but takes the JSON payload
> +  percent-encoded to ease passing around this option without getting the
> +  JSON payload corrupted.
> +
>  Changes in 2.42:
>
>  * Add -z mark-plt/-z nomark-plt options to x86-64 ELF linker to mark PLT
> diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
> index 863657e12f5..1a893f35416 100644
> --- 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,24 @@ 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);
> +      ldelf_emit_note_fdo_package_metadata = NULL;
> +      if (optarg != NULL)
> +       {
> +         size_t optarg_len = strlen (optarg);
> +         if (optarg_len > 0)
> +           {
> +             char *package_metadata = xmalloc (optarg_len + 1);
> +             int len = percent_decode (optarg, package_metadata);
> +             if (len < 0)
> +               einfo (_("%F%P: Failed to decode percent-encoded package"
> +                        " metadata \`%s'\n"), optarg);
> +             ldelf_emit_note_fdo_package_metadata = package_metadata;
> +           }
> +        }
> +      break;
> +
>      case OPTION_PACKAGE_METADATA:
>        free ((char *) ldelf_emit_note_fdo_package_metadata);
>        ldelf_emit_note_fdo_package_metadata = NULL;
> diff --git a/ld/ld.texi b/ld/ld.texi
> index 89e3913317a..28733b1ae8e 100644
> --- a/ld/ld.texi
> +++ b/ld/ld.texi
> @@ -3234,6 +3234,19 @@ string identifying the original linked file does
> not change.
>  Passing @code{none} for @var{style} disables the setting from any
>  @code{--build-id} options earlier on the command line.
>
> +@kindex --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
> +@item --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
> +Request the creation of a @code{.note.package} ELF note section.  The
> +contents of the note are in JSON format, as per the package metadata
> +specification.  For more information see:
> +https://systemd.io/ELF_PACKAGE_METADATA/
> +If the PERCENT_ENCODED_JSON argument is missing/empty then this will
> +disable the creation of the metadata note, if one had been enabled by
> +an earlier occurrence of the --encoded-package-metadata or
> +--package-metadata option. PERCENT_ENCODED_JSON will be percent-decoded.
> +If the linker has been built with libjansson, then the JSON string
> +will be validated.
> +
>  @kindex --package-metadata=@var{JSON}
>  @item --package-metadata=@var{JSON}
>  Request the creation of a @code{.note.package} ELF note section.  The
> @@ -3242,7 +3255,7 @@ specification.  For more information see:
>  https://systemd.io/ELF_PACKAGE_METADATA/
>  If the JSON argument is missing/empty then this will disable the
>  creation of the metadata note, if one had been enabled by an earlier
> -occurrence of the --package-metadata option.
> +occurrence of the --encoded-package-metadata or --package-metadata option.
>  If the linker has been built with libjansson, then the JSON string
>  will be validated.
>  @end table
> diff --git a/ld/ldlex.h b/ld/ldlex.h
> index defe3fcbbb9..225d95b1e90 100644
> --- a/ld/ldlex.h
> +++ b/ld/ldlex.h
> @@ -284,6 +284,7 @@ enum option_values
>    OPTION_EH_FRAME_HDR,
>    OPTION_NO_EH_FRAME_HDR,
>    OPTION_HASH_STYLE,
> +  OPTION_ENCODED_PACKAGE_METADATA,
>    OPTION_PACKAGE_METADATA,
>    OPTION_AUDIT,
>    OPTION_COMPRESS_DEBUG,
> diff --git a/ld/ldmisc.c b/ld/ldmisc.c
> index 180b24b3448..ff98226afd6 100644
> --- a/ld/ldmisc.c
> +++ b/ld/ldmisc.c
> @@ -770,3 +770,52 @@ 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 (const char *src, char *dst)
> +{
> +  int length = 0;
> +  while (*src != '\0')
> +    {
> +      char c = *src++;
> +      if (c != '%')
> +       {
> +         *dst++ = c;
> +         length += 1;
> +         continue;
> +       }
> +      char next1 = *src++;
> +      if (next1 == '%')
> +       {
> +         *dst++ = '%';
> +         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) + hex2);
> +      length += 1;
> +    }
> +  *dst = '\0';
> +  return length;
> +}
> diff --git a/ld/ldmisc.h b/ld/ldmisc.h
> index 20289127c0a..815e8ccb548 100644
> --- a/ld/ldmisc.h
> +++ b/ld/ldmisc.h
> @@ -39,5 +39,6 @@ do { info_assert(__FILE__,__LINE__); } while (0)
>  extern void print_spaces (int);
>  #define print_space() print_spaces (1)
>  extern void print_nl (void);
> +extern int percent_decode (const char *, char *);
>
>  #endif
> diff --git a/ld/lexsup.c b/ld/lexsup.c
> index 4aa0124ce2f..60e329a713f 100644
> --- a/ld/lexsup.c
> +++ b/ld/lexsup.c
> @@ -2279,6 +2279,8 @@ elf_static_list_options (FILE *file)
>    fprintf (file, _("\
>    --build-id[=STYLE]          Generate build ID note\n"));
>    fprintf (file, _("\
> +  --encoded-package-metadata[=PERCENT_ENCODED_JSON]   Generate package
> metadata note\n"));
> +  fprintf (file, _("\
>    --package-metadata[=JSON]   Generate package metadata note\n"));
>    fprintf (file, _("\
>    --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi|zstd]\n\
> diff --git a/ld/testsuite/ld-elf/package-note.exp
> b/ld/testsuite/ld-elf/package-note.exp
> index 9f18705bd65..b4feef541b4 100644
> --- a/ld/testsuite/ld-elf/package-note.exp
> +++ b/ld/testsuite/ld-elf/package-note.exp
> @@ -42,4 +42,22 @@ run_ld_link_tests [list \
>         {{readelf {--notes} package-note.rd}} \
>         "package-note.o" \
>      ] \
> +    [list \
> +       "package-note1b.o" \
> +       "--encoded-package-metadata=%7B%22foo%22%3A%22bar%22%7D" \
> +       "" \
> +       "" \
> +       {start.s} \
> +       {{readelf {--notes} package-note.rd}} \
> +       "package-note1b.o" \
> +    ] \
> +    [list \
> +       "package-note2.o" \
> +
>  "--encoded-package-metadata=%7B%22name%22:%22binutils%22%2C%22foo%22%3A%22bar%22%7d"
> \
> +       "" \
> +       "" \
> +       {start.s} \
> +       {{readelf {--notes} package-note2.rd}} \
> +       "package-note2.o" \
> +    ] \
>  ]
> diff --git a/ld/testsuite/ld-elf/package-note2.rd
> b/ld/testsuite/ld-elf/package-note2.rd
> new file mode 100644
> index 00000000000..c24c37825ad
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/package-note2.rd
> @@ -0,0 +1,6 @@
> +#...
> +Displaying notes found in: \.note\.package
> +\s+Owner\s+Data\s+size\s+Description
> +\s+FDO\s+0x00000020\s+(Unknown note
> type:\s+\(0xcafe1a7e\)|FDO_PACKAGING_METADATA)
> +\s+(description data:\s+.*|Packaging
> Metadata:\s+{"name":"binutils","foo":"bar"})
> +#pass
> --
> 2.43.0
>
>
>

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

[Jan Beulich]

On 21.07.2024 00:34, H.J. Lu wrote:
> On Thu, Jul 18, 2024, 5:05 PM Benjamin Drung <benjamin.drung@canonical.com>
> 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.
> 
> Please open a bug report for this feature request.
> A test case is needed to show why this feature is needed.

There is a testcase now (there wasn't in the initial version, iirc). I don't,
however, see how a testcase can "show why this feature is needed". The need
is supplied in the patch description to an imo sufficient degree.

Jan

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

[Benjamin Drung]

On Sun, 2024-07-21 at 06:34 +0800, H.J. Lu wrote:
> 
> On Thu, Jul 18, 2024, 5:05 PM Benjamin Drung <benjamin.drung@canonical.com> 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.
> 
> Please open a bug report for this feature request.
> A test case is needed to show why this feature is needed.


I created https://sourceware.org/bugzilla/show_bug.cgi?id=32003 for it
now.

-- 
Benjamin Drung
Debian & Ubuntu Developer

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

[Jan Beulich]

On 18.07.2024 11:04, 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>

Okay. Please don't forget to add a reference to the PR that you created
upon H.J.'s request. I notice H.J. has voiced a readability concern
there, but I think that can be taken care of incrementally.

Cc-ing Nick as to views towards putting this on the branch as well.

Jan

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

On Tue, Jul 23, 2024, 6:19 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 18.07.2024 11:04, 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>
>
> Okay. Please don't forget to add a reference to the PR that you created
> upon H.J.'s request. I notice H.J. has voiced a readability concern
> there, but I think that can be taken care of incrementally.
>

I think we should make the existing option
usable instead of adding another option.


> Cc-ing Nick as to views towards putting this on the branch as well.
>
> Jan
>
>

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

[Jan Beulich]

On 23.07.2024 12:24, H.J. Lu wrote:
> On Tue, Jul 23, 2024, 6:19 PM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> On 18.07.2024 11:04, 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>
>>
>> Okay. Please don't forget to add a reference to the PR that you created
>> upon H.J.'s request. I notice H.J. has voiced a readability concern
>> there, but I think that can be taken care of incrementally.
> 
> I think we should make the existing option
> usable instead of adding another option.

Can you at least vaguely explain how you envision doing so, without any
kind of risk of breaking existing uses?

Jan

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

On Tue, Jul 23, 2024, 6:39 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 23.07.2024 12:24, H.J. Lu wrote:
> > On Tue, Jul 23, 2024, 6:19 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> >> On 18.07.2024 11:04, 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>
> >>
> >> Okay. Please don't forget to add a reference to the PR that you created
> >> upon H.J.'s request. I notice H.J. has voiced a readability concern
> >> there, but I think that can be taken care of incrementally.
> >
> > I think we should make the existing option
> > usable instead of adding another option.
>
> Can you at least vaguely explain how you envision doing so, without any
> kind of risk of breaking existing uses?
>

See my comments in the bug report.


> Jan
>
>

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

[Benjamin Drung]

Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
work in case the JSON contains a comma, because compiler drivers eat
commas. Example:

```
$ echo "void main() { }" > test.c
$ gcc '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
/usr/bin/ld: cannot find "os":"ubuntu"}: No such file or directory
collect2: error: ld returned 1 exit status
```

The quotation marks in the JSON value do not work well with shell nor
make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
environment variable might loose its quotation marks when it hits the
final compiler call.

So introduce a `--encoded-package-metadata` linker flag that takes a
percent-encoded JSON. Percent-encoding is used because it is a
standard, simple to implement, and does take too many additional
characters.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
---
 ld/NEWS                              |  5 +++
 ld/emultempl/elf.em                  | 19 +++++++++++
 ld/ld.texi                           | 15 ++++++++-
 ld/ldlex.h                           |  1 +
 ld/ldmisc.c                          | 49 ++++++++++++++++++++++++++++
 ld/ldmisc.h                          |  1 +
 ld/lexsup.c                          |  2 ++
 ld/testsuite/ld-elf/package-note.exp | 18 ++++++++++
 ld/testsuite/ld-elf/package-note2.rd |  6 ++++
 9 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 ld/testsuite/ld-elf/package-note2.rd

diff --git a/ld/NEWS b/ld/NEWS
index c1e42139b8f..a1880ccf1bb 100644
--- a/ld/NEWS
+++ b/ld/NEWS
@@ -14,6 +14,11 @@ Changes in 2.43:
 
 * Add -plugin-save-temps to store plugin intermediate files permanently.
 
+* The ELF linker now supports a new --encoded-package-metadata option that
+  is equivalent to --package-metadata, but takes the JSON payload
+  percent-encoded to ease passing around this option without getting the
+  JSON payload corrupted.
+
 Changes in 2.42:
 
 * Add -z mark-plt/-z nomark-plt options to x86-64 ELF linker to mark PLT
diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
index 863657e12f5..1a893f35416 100644
--- 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,24 @@ 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);
+      ldelf_emit_note_fdo_package_metadata = NULL;
+      if (optarg != NULL)
+	{
+	  size_t optarg_len = strlen (optarg);
+	  if (optarg_len > 0)
+	    {
+	      char *package_metadata = xmalloc (optarg_len + 1);
+	      int len = percent_decode (optarg, package_metadata);
+	      if (len < 0)
+		einfo (_("%F%P: Failed to decode percent-encoded package"
+		         " metadata \`%s'\n"), optarg);
+	      ldelf_emit_note_fdo_package_metadata = package_metadata;
+	    }
+        }
+      break;
+
     case OPTION_PACKAGE_METADATA:
       free ((char *) ldelf_emit_note_fdo_package_metadata);
       ldelf_emit_note_fdo_package_metadata = NULL;
diff --git a/ld/ld.texi b/ld/ld.texi
index 89e3913317a..28733b1ae8e 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -3234,6 +3234,19 @@ string identifying the original linked file does not change.
 Passing @code{none} for @var{style} disables the setting from any
 @code{--build-id} options earlier on the command line.
 
+@kindex --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+@item --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
+Request the creation of a @code{.note.package} ELF note section.  The
+contents of the note are in JSON format, as per the package metadata
+specification.  For more information see:
+https://systemd.io/ELF_PACKAGE_METADATA/
+If the PERCENT_ENCODED_JSON argument is missing/empty then this will
+disable the creation of the metadata note, if one had been enabled by
+an earlier occurrence of the --encoded-package-metadata or
+--package-metadata option. PERCENT_ENCODED_JSON will be percent-decoded.
+If the linker has been built with libjansson, then the JSON string
+will be validated.
+
 @kindex --package-metadata=@var{JSON}
 @item --package-metadata=@var{JSON}
 Request the creation of a @code{.note.package} ELF note section.  The
@@ -3242,7 +3255,7 @@ specification.  For more information see:
 https://systemd.io/ELF_PACKAGE_METADATA/
 If the JSON argument is missing/empty then this will disable the
 creation of the metadata note, if one had been enabled by an earlier
-occurrence of the --package-metadata option.
+occurrence of the --encoded-package-metadata or --package-metadata option.
 If the linker has been built with libjansson, then the JSON string
 will be validated.
 @end table
diff --git a/ld/ldlex.h b/ld/ldlex.h
index defe3fcbbb9..225d95b1e90 100644
--- a/ld/ldlex.h
+++ b/ld/ldlex.h
@@ -284,6 +284,7 @@ enum option_values
   OPTION_EH_FRAME_HDR,
   OPTION_NO_EH_FRAME_HDR,
   OPTION_HASH_STYLE,
+  OPTION_ENCODED_PACKAGE_METADATA,
   OPTION_PACKAGE_METADATA,
   OPTION_AUDIT,
   OPTION_COMPRESS_DEBUG,
diff --git a/ld/ldmisc.c b/ld/ldmisc.c
index 180b24b3448..ff98226afd6 100644
--- a/ld/ldmisc.c
+++ b/ld/ldmisc.c
@@ -770,3 +770,52 @@ 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 (const char *src, char *dst)
+{
+  int length = 0;
+  while (*src != '\0')
+    {
+      char c = *src++;
+      if (c != '%')
+	{
+	  *dst++ = c;
+	  length += 1;
+	  continue;
+	}
+      char next1 = *src++;
+      if (next1 == '%')
+	{
+	  *dst++ = '%';
+	  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) + hex2);
+      length += 1;
+    }
+  *dst = '\0';
+  return length;
+}
diff --git a/ld/ldmisc.h b/ld/ldmisc.h
index 20289127c0a..815e8ccb548 100644
--- a/ld/ldmisc.h
+++ b/ld/ldmisc.h
@@ -39,5 +39,6 @@ do { info_assert(__FILE__,__LINE__); } while (0)
 extern void print_spaces (int);
 #define print_space() print_spaces (1)
 extern void print_nl (void);
+extern int percent_decode (const char *, char *);
 
 #endif
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 4aa0124ce2f..60e329a713f 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -2279,6 +2279,8 @@ elf_static_list_options (FILE *file)
   fprintf (file, _("\
   --build-id[=STYLE]          Generate build ID note\n"));
   fprintf (file, _("\
+  --encoded-package-metadata[=PERCENT_ENCODED_JSON]   Generate package metadata note\n"));
+  fprintf (file, _("\
   --package-metadata[=JSON]   Generate package metadata note\n"));
   fprintf (file, _("\
   --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi|zstd]\n\
diff --git a/ld/testsuite/ld-elf/package-note.exp b/ld/testsuite/ld-elf/package-note.exp
index 9f18705bd65..b4feef541b4 100644
--- a/ld/testsuite/ld-elf/package-note.exp
+++ b/ld/testsuite/ld-elf/package-note.exp
@@ -42,4 +42,22 @@ run_ld_link_tests [list \
 	{{readelf {--notes} package-note.rd}} \
 	"package-note.o" \
     ] \
+    [list \
+	"package-note1b.o" \
+	"--encoded-package-metadata=%7B%22foo%22%3A%22bar%22%7D" \
+	"" \
+	"" \
+	{start.s} \
+	{{readelf {--notes} package-note.rd}} \
+	"package-note1b.o" \
+    ] \
+    [list \
+	"package-note2.o" \
+	"--encoded-package-metadata=%7B%22name%22:%22binutils%22%2C%22foo%22%3A%22bar%22%7d" \
+	"" \
+	"" \
+	{start.s} \
+	{{readelf {--notes} package-note2.rd}} \
+	"package-note2.o" \
+    ] \
 ]
diff --git a/ld/testsuite/ld-elf/package-note2.rd b/ld/testsuite/ld-elf/package-note2.rd
new file mode 100644
index 00000000000..c24c37825ad
--- /dev/null
+++ b/ld/testsuite/ld-elf/package-note2.rd
@@ -0,0 +1,6 @@
+#...
+Displaying notes found in: \.note\.package
+\s+Owner\s+Data\s+size\s+Description
+\s+FDO\s+0x00000020\s+(Unknown note type:\s+\(0xcafe1a7e\)|FDO_PACKAGING_METADATA)
+\s+(description data:\s+.*|Packaging Metadata:\s+{"name":"binutils","foo":"bar"})
+#pass
-- 
2.43.0

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 10231 bytes --]

On Wed, Jul 24, 2024, 7:29 AM Benjamin Drung <benjamin.drung@canonical.com>
wrote:

> Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
> work in case the JSON contains a comma, because compiler drivers eat
> commas. Example:
>
> ```
> $ echo "void main() { }" > test.c
> $ gcc '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
> /usr/bin/ld: cannot find "os":"ubuntu"}: No such file or directory
> collect2: error: ld returned 1 exit status
> ```
>
> The quotation marks in the JSON value do not work well with shell nor
> make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
> environment variable might loose its quotation marks when it hits the
> final compiler call.
>
> So introduce a `--encoded-package-metadata` linker flag that takes a
>

I think `--package-metadata` should be updated
with escape support if it doesn't introduce regressions.

percent-encoded JSON. Percent-encoding is used because it is a
> standard, simple to implement, and does take too many additional
> characters.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
> Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
> ---
>  ld/NEWS                              |  5 +++
>  ld/emultempl/elf.em                  | 19 +++++++++++
>  ld/ld.texi                           | 15 ++++++++-
>  ld/ldlex.h                           |  1 +
>  ld/ldmisc.c                          | 49 ++++++++++++++++++++++++++++
>  ld/ldmisc.h                          |  1 +
>  ld/lexsup.c                          |  2 ++
>  ld/testsuite/ld-elf/package-note.exp | 18 ++++++++++
>  ld/testsuite/ld-elf/package-note2.rd |  6 ++++
>  9 files changed, 115 insertions(+), 1 deletion(-)
>  create mode 100644 ld/testsuite/ld-elf/package-note2.rd
>
> diff --git a/ld/NEWS b/ld/NEWS
> index c1e42139b8f..a1880ccf1bb 100644
> --- a/ld/NEWS
> +++ b/ld/NEWS
> @@ -14,6 +14,11 @@ Changes in 2.43:
>
>  * Add -plugin-save-temps to store plugin intermediate files permanently.
>
> +* The ELF linker now supports a new --encoded-package-metadata option that
> +  is equivalent to --package-metadata, but takes the JSON payload
> +  percent-encoded to ease passing around this option without getting the
> +  JSON payload corrupted.
> +
>  Changes in 2.42:
>
>  * Add -z mark-plt/-z nomark-plt options to x86-64 ELF linker to mark PLT
> diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
> index 863657e12f5..1a893f35416 100644
> --- 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,24 @@ 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);
> +      ldelf_emit_note_fdo_package_metadata = NULL;
> +      if (optarg != NULL)
> +       {
> +         size_t optarg_len = strlen (optarg);
> +         if (optarg_len > 0)
> +           {
> +             char *package_metadata = xmalloc (optarg_len + 1);
> +             int len = percent_decode (optarg, package_metadata);
> +             if (len < 0)
> +               einfo (_("%F%P: Failed to decode percent-encoded package"
> +                        " metadata \`%s'\n"), optarg);
> +             ldelf_emit_note_fdo_package_metadata = package_metadata;
> +           }
> +        }
> +      break;
> +
>      case OPTION_PACKAGE_METADATA:
>        free ((char *) ldelf_emit_note_fdo_package_metadata);
>        ldelf_emit_note_fdo_package_metadata = NULL;
> diff --git a/ld/ld.texi b/ld/ld.texi
> index 89e3913317a..28733b1ae8e 100644
> --- a/ld/ld.texi
> +++ b/ld/ld.texi
> @@ -3234,6 +3234,19 @@ string identifying the original linked file does
> not change.
>  Passing @code{none} for @var{style} disables the setting from any
>  @code{--build-id} options earlier on the command line.
>
> +@kindex --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
> +@item --encoded-package-metadata=@var{PERCENT_ENCODED_JSON}
> +Request the creation of a @code{.note.package} ELF note section.  The
> +contents of the note are in JSON format, as per the package metadata
> +specification.  For more information see:
> +https://systemd.io/ELF_PACKAGE_METADATA/
> +If the PERCENT_ENCODED_JSON argument is missing/empty then this will
> +disable the creation of the metadata note, if one had been enabled by
> +an earlier occurrence of the --encoded-package-metadata or
> +--package-metadata option. PERCENT_ENCODED_JSON will be percent-decoded.
> +If the linker has been built with libjansson, then the JSON string
> +will be validated.
> +
>  @kindex --package-metadata=@var{JSON}
>  @item --package-metadata=@var{JSON}
>  Request the creation of a @code{.note.package} ELF note section.  The
> @@ -3242,7 +3255,7 @@ specification.  For more information see:
>  https://systemd.io/ELF_PACKAGE_METADATA/
>  If the JSON argument is missing/empty then this will disable the
>  creation of the metadata note, if one had been enabled by an earlier
> -occurrence of the --package-metadata option.
> +occurrence of the --encoded-package-metadata or --package-metadata option.
>  If the linker has been built with libjansson, then the JSON string
>  will be validated.
>  @end table
> diff --git a/ld/ldlex.h b/ld/ldlex.h
> index defe3fcbbb9..225d95b1e90 100644
> --- a/ld/ldlex.h
> +++ b/ld/ldlex.h
> @@ -284,6 +284,7 @@ enum option_values
>    OPTION_EH_FRAME_HDR,
>    OPTION_NO_EH_FRAME_HDR,
>    OPTION_HASH_STYLE,
> +  OPTION_ENCODED_PACKAGE_METADATA,
>    OPTION_PACKAGE_METADATA,
>    OPTION_AUDIT,
>    OPTION_COMPRESS_DEBUG,
> diff --git a/ld/ldmisc.c b/ld/ldmisc.c
> index 180b24b3448..ff98226afd6 100644
> --- a/ld/ldmisc.c
> +++ b/ld/ldmisc.c
> @@ -770,3 +770,52 @@ 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 (const char *src, char *dst)
> +{
> +  int length = 0;
> +  while (*src != '\0')
> +    {
> +      char c = *src++;
> +      if (c != '%')
> +       {
> +         *dst++ = c;
> +         length += 1;
> +         continue;
> +       }
> +      char next1 = *src++;
> +      if (next1 == '%')
> +       {
> +         *dst++ = '%';
> +         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) + hex2);
> +      length += 1;
> +    }
> +  *dst = '\0';
> +  return length;
> +}
> diff --git a/ld/ldmisc.h b/ld/ldmisc.h
> index 20289127c0a..815e8ccb548 100644
> --- a/ld/ldmisc.h
> +++ b/ld/ldmisc.h
> @@ -39,5 +39,6 @@ do { info_assert(__FILE__,__LINE__); } while (0)
>  extern void print_spaces (int);
>  #define print_space() print_spaces (1)
>  extern void print_nl (void);
> +extern int percent_decode (const char *, char *);
>
>  #endif
> diff --git a/ld/lexsup.c b/ld/lexsup.c
> index 4aa0124ce2f..60e329a713f 100644
> --- a/ld/lexsup.c
> +++ b/ld/lexsup.c
> @@ -2279,6 +2279,8 @@ elf_static_list_options (FILE *file)
>    fprintf (file, _("\
>    --build-id[=STYLE]          Generate build ID note\n"));
>    fprintf (file, _("\
> +  --encoded-package-metadata[=PERCENT_ENCODED_JSON]   Generate package
> metadata note\n"));
> +  fprintf (file, _("\
>    --package-metadata[=JSON]   Generate package metadata note\n"));
>    fprintf (file, _("\
>    --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi|zstd]\n\
> diff --git a/ld/testsuite/ld-elf/package-note.exp
> b/ld/testsuite/ld-elf/package-note.exp
> index 9f18705bd65..b4feef541b4 100644
> --- a/ld/testsuite/ld-elf/package-note.exp
> +++ b/ld/testsuite/ld-elf/package-note.exp
> @@ -42,4 +42,22 @@ run_ld_link_tests [list \
>         {{readelf {--notes} package-note.rd}} \
>         "package-note.o" \
>      ] \
> +    [list \
> +       "package-note1b.o" \
> +       "--encoded-package-metadata=%7B%22foo%22%3A%22bar%22%7D" \
> +       "" \
> +       "" \
> +       {start.s} \
> +       {{readelf {--notes} package-note.rd}} \
> +       "package-note1b.o" \
> +    ] \
> +    [list \
> +       "package-note2.o" \
> +
>  "--encoded-package-metadata=%7B%22name%22:%22binutils%22%2C%22foo%22%3A%22bar%22%7d"
> \
> +       "" \
> +       "" \
> +       {start.s} \
> +       {{readelf {--notes} package-note2.rd}} \
> +       "package-note2.o" \
> +    ] \
>  ]
> diff --git a/ld/testsuite/ld-elf/package-note2.rd
> b/ld/testsuite/ld-elf/package-note2.rd
> new file mode 100644
> index 00000000000..c24c37825ad
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/package-note2.rd
> @@ -0,0 +1,6 @@
> +#...
> +Displaying notes found in: \.note\.package
> +\s+Owner\s+Data\s+size\s+Description
> +\s+FDO\s+0x00000020\s+(Unknown note
> type:\s+\(0xcafe1a7e\)|FDO_PACKAGING_METADATA)
> +\s+(description data:\s+.*|Packaging
> Metadata:\s+{"name":"binutils","foo":"bar"})
> +#pass
> --
> 2.43.0
>
>

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

[Jan Beulich]

On 24.07.2024 01:28, Benjamin Drung wrote:
> Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
> work in case the JSON contains a comma, because compiler drivers eat
> commas. Example:
> 
> ```
> $ echo "void main() { }" > test.c
> $ gcc '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
> /usr/bin/ld: cannot find "os":"ubuntu"}: No such file or directory
> collect2: error: ld returned 1 exit status
> ```
> 
> The quotation marks in the JSON value do not work well with shell nor
> make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
> environment variable might loose its quotation marks when it hits the
> final compiler call.
> 
> So introduce a `--encoded-package-metadata` linker flag that takes a
> percent-encoded JSON. Percent-encoding is used because it is a
> standard, simple to implement, and does take too many additional
> characters.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
> Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
> ---
>  ld/NEWS                              |  5 +++
>  ld/emultempl/elf.em                  | 19 +++++++++++
>  ld/ld.texi                           | 15 ++++++++-
>  ld/ldlex.h                           |  1 +
>  ld/ldmisc.c                          | 49 ++++++++++++++++++++++++++++
>  ld/ldmisc.h                          |  1 +
>  ld/lexsup.c                          |  2 ++
>  ld/testsuite/ld-elf/package-note.exp | 18 ++++++++++
>  ld/testsuite/ld-elf/package-note2.rd |  6 ++++
>  9 files changed, 115 insertions(+), 1 deletion(-)
>  create mode 100644 ld/testsuite/ld-elf/package-note2.rd

And what exactly has changed in v5? There's no revlog anywhere here,
afaics.

Jan

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

[Benjamin Drung]

On Wed, 2024-07-24 at 07:48 +0200, Jan Beulich wrote:
> On 24.07.2024 01:28, Benjamin Drung wrote:
> > Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
> > work in case the JSON contains a comma, because compiler drivers eat
> > commas. Example:
> > 
> > ```
> > $ echo "void main() { }" > test.c
> > $ gcc '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
> > /usr/bin/ld: cannot find "os":"ubuntu"}: No such file or directory
> > collect2: error: ld returned 1 exit status
> > ```
> > 
> > The quotation marks in the JSON value do not work well with shell nor
> > make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
> > environment variable might loose its quotation marks when it hits the
> > final compiler call.
> > 
> > So introduce a `--encoded-package-metadata` linker flag that takes a
> > percent-encoded JSON. Percent-encoding is used because it is a
> > standard, simple to implement, and does take too many additional
> > characters.
> > 
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
> > Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> > Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
> > ---
> >  ld/NEWS                              |  5 +++
> >  ld/emultempl/elf.em                  | 19 +++++++++++
> >  ld/ld.texi                           | 15 ++++++++-
> >  ld/ldlex.h                           |  1 +
> >  ld/ldmisc.c                          | 49 ++++++++++++++++++++++++++++
> >  ld/ldmisc.h                          |  1 +
> >  ld/lexsup.c                          |  2 ++
> >  ld/testsuite/ld-elf/package-note.exp | 18 ++++++++++
> >  ld/testsuite/ld-elf/package-note2.rd |  6 ++++
> >  9 files changed, 115 insertions(+), 1 deletion(-)
> >  create mode 100644 ld/testsuite/ld-elf/package-note2.rd
> 
> And what exactly has changed in v5? There's no revlog anywhere here,
> afaics.

v2 reformats the code.

v3 adds test cases and addresses compiler warnings.

v4 makes the %% parsing code path a little bit more readable.

v5 is v4 with updated commit description (including referencing the open
bug).

-- 
Benjamin Drung
Debian & Ubuntu Developer

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

[Nick Clifton]

Hi Jan, Hi Benjamin, Hi Matthias

>> 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.
> 
> Okay. Please don't forget to add a reference to the PR that you created
> upon H.J.'s request. I notice H.J. has voiced a readability concern
> there, but I think that can be taken care of incrementally.
> 
> Cc-ing Nick as to views towards putting this on the branch as well.

Hmm, I am open to the idea, but only if the commit (to the mainline)
happens this week.  That will give me enough time to backport the
patch and test it on the 2.43 branch.

Cheers
   Nick

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

On Thu, Jul 25, 2024, 6:00 PM Nick Clifton <nickc@redhat.com> wrote:

> Hi Jan, Hi Benjamin, Hi Matthias
>
> >> 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.
> >
> > Okay. Please don't forget to add a reference to the PR that you created
> > upon H.J.'s request. I notice H.J. has voiced a readability concern
> > there, but I think that can be taken care of incrementally.
> >
> > Cc-ing Nick as to views towards putting this on the branch as well.
>
> Hmm, I am open to the idea, but only if the commit (to the mainline)
> happens this week.  That will give me enough time to backport the
> patch and test it on the 2.43 branch.
>

I don't think this option is human readable.
It is a bad approach.


> Cheers
>    Nick
>
>
>
>

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

[Matthias Klose]

On 25.07.24 12:35, H.J. Lu wrote:
> On Thu, Jul 25, 2024, 6:00 PM Nick Clifton <nickc@redhat.com> wrote:
> 
>> Hi Jan, Hi Benjamin, Hi Matthias
>>
>>>> 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.
>>>
>>> Okay. Please don't forget to add a reference to the PR that you created
>>> upon H.J.'s request. I notice H.J. has voiced a readability concern
>>> there, but I think that can be taken care of incrementally.
>>>
>>> Cc-ing Nick as to views towards putting this on the branch as well.
>>
>> Hmm, I am open to the idea, but only if the commit (to the mainline)
>> happens this week.  That will give me enough time to backport the
>> patch and test it on the 2.43 branch.
>>
> 
> I don't think this option is human readable.
> It is a bad approach.

does it have to be human readable?  This option is always auto-generated 
by some packaging system.  The value is checked in the linkers to be a 
valid json file. What is needed is a way to propagate that information 
down to the linker.

  - The current approach taken by Fedora to pass a --specs option
    to the compiler, and constructing the --package-metadata
    in a new link spec using the getenv function for the GCC specs
    doesn't work
     + for compilers other than GCC
     + for debugging, when not every env var required in the
       specs file is set, and the compiler bailing out with
       an error. You would have to remove the --specs option
       from generated Makefiles, and then dealing with
       the build system rebuilding these generated Makefiles.

  - The current option --package-metadata is not good enough,
    it's too fragile with the characters interpreted by various
    shells and build systems, also options are saved in some
    way in build artifacts.  You cannot assume to fix/change
    every build system.
    I assume the Fedora people saw that as well, and then
    introduced the new specs.

  - --encoded-package-metadata tries to resolve this by making the
    option values robust. Sure, you can go with the existing option
    and accepting some encoded option value as well. I wouldn't care
    that much about compatibility, because the injection of that
    option in the build system and having that option available in
    the linkers is almost always in the hands of the same people,
    e.g distribution maintainers.

  - I want to go further for cases, where the option is lost in
    some place, the build system not passing that option down to
    the linker for some reason.
    For these cases I consider to have something similar like
    the SOURCE_DATE_EPOCH envvar, like ELF_PACKAGE_METADATA,
    and the linkers picking up this envvar if it exists, and if
    no package-metadata option is given.  I am currently thinking
    to have this as a local patch in the distros, but if the
    upstreams think that is useful to have upstream, I'll
    submit patches for this as well.

    This alternative pics up the metadata information from the
    environment in one piece, not like the Fedora approach of
    picking up five different envvars in the specs file, and then
    failing in the compiler if something is unset.

I don't think, that many people have been asked or involved with this 
metadata thing, and how this information is supposed to get there.  It 
seems mostly a Fedora/Systemd thing.  Benjamin is currently trying to 
get that information into Ubuntu, and I'm also trying to get this into 
Debian, when we see a viable approach without too many glitches.

Matthias

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

[Luca Boccassi]

>   - --encoded-package-metadata tries to resolve this by making the
>     option values robust. Sure, you can go with the existing option
>     and accepting some encoded option value as well. I wouldn't care
>     that much about compatibility, because the injection of that
>     option in the build system and having that option available in
>     the linkers is almost always in the hands of the same people,
>     e.g distribution maintainers.

Speaking as the author of the original option, and current user (in
Fedora where I helped integrate it, in Debian where I set it up as opt-
in and used by many packages, and at $work), adding a new option to
make it work nicely in more settings is of course absolutely fine.
Changing the existing option to also take encoded input is also
absolutely fine, no issue whatsoever. Adding an env var as an
alternative, also great, no problem at all.

Breaking backward compatibility is not fine, however. This is
functionality present in multiple binutils stable releases, it is
stable, it works, and it is currently used widely, so breaking backward
compatibility is not acceptable, as it will break many production use
cases, so please do not do that. Again anything else as mentioned above
is great to have.

> I don't think, that many people have been asked or involved with this
> metadata thing, and how this information is supposed to get there. 
> It 
> seems mostly a Fedora/Systemd thing.  Benjamin is currently trying to
> get that information into Ubuntu

I am very happy that this is being implemented in Ubuntu, and I am
grateful to everybody for that doing work. But here you seem to imply
that this was somehow silently sneaked in, and I don't think that's
fair. It was discussed at length in many different forums - in this and
other linker's MLs/repos at implementation time, and before that on
Debian's mailing list and on the Fedora's change proposal. We even
presented on this work at Packagingcon some years ago. I'm not sure
what it could have been done to make it even "more" public, there was
an LWN article too: https://lwn.net/Articles/874642/

> I'm also trying to get this into Debian

It is already in Debian, as opt-in. The dpkg maintainer flat out
rejected the whole concept, even before there was any semblance of an
implementation, and the debhelper maintainer understandably does not
want his build tool to become the dumping ground for dpkg workarounds,
so in Debian it will just have to remain opt-in unfortunately. It has
already been there as such, in Debian Stable too, and it works just
fine, the opt in is one build dep and one include line in debian/rules.

-- 
Kind regards,
Luca Boccassi

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

[Benjamin Drung]

On Sat, 2024-07-27 at 08:27 +0200, Matthias Klose wrote:
> On 25.07.24 12:35, H.J. Lu wrote:
> > On Thu, Jul 25, 2024, 6:00 PM Nick Clifton <nickc@redhat.com> wrote:
> > 
> > > Hi Jan, Hi Benjamin, Hi Matthias
> > > 
> > > > > 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.
> > > > 
> > > > Okay. Please don't forget to add a reference to the PR that you created
> > > > upon H.J.'s request. I notice H.J. has voiced a readability concern
> > > > there, but I think that can be taken care of incrementally.
> > > > 
> > > > Cc-ing Nick as to views towards putting this on the branch as well.
> > > 
> > > Hmm, I am open to the idea, but only if the commit (to the mainline)
> > > happens this week.  That will give me enough time to backport the
> > > patch and test it on the 2.43 branch.
> > > 
> > 
> > I don't think this option is human readable.
> > It is a bad approach.
> 
> does it have to be human readable?  This option is always auto-generated 
> by some packaging system.  The value is checked in the linkers to be a 
> valid json file. What is needed is a way to propagate that information 
> down to the linker.
> 
>   - The current approach taken by Fedora to pass a --specs option
>     to the compiler, and constructing the --package-metadata
>     in a new link spec using the getenv function for the GCC specs
>     doesn't work
>      + for compilers other than GCC
>      + for debugging, when not every env var required in the
>        specs file is set, and the compiler bailing out with
>        an error. You would have to remove the --specs option
>        from generated Makefiles, and then dealing with
>        the build system rebuilding these generated Makefiles.
> 
>   - The current option --package-metadata is not good enough,
>     it's too fragile with the characters interpreted by various
>     shells and build systems, also options are saved in some
>     way in build artifacts.  You cannot assume to fix/change
>     every build system.
>     I assume the Fedora people saw that as well, and then
>     introduced the new specs.
> 
>   - --encoded-package-metadata tries to resolve this by making the
>     option values robust. Sure, you can go with the existing option
>     and accepting some encoded option value as well. I wouldn't care
>     that much about compatibility, because the injection of that
>     option in the build system and having that option available in
>     the linkers is almost always in the hands of the same people,
>     e.g distribution maintainers.
> 
>   - I want to go further for cases, where the option is lost in
>     some place, the build system not passing that option down to
>     the linker for some reason.
>     For these cases I consider to have something similar like
>     the SOURCE_DATE_EPOCH envvar, like ELF_PACKAGE_METADATA,
>     and the linkers picking up this envvar if it exists, and if
>     no package-metadata option is given.  I am currently thinking
>     to have this as a local patch in the distros, but if the
>     upstreams think that is useful to have upstream, I'll
>     submit patches for this as well.
> 
>     This alternative pics up the metadata information from the
>     environment in one piece, not like the Fedora approach of
>     picking up five different envvars in the specs file, and then
>     failing in the compiler if something is unset.
> 
> I don't think, that many people have been asked or involved with this 
> metadata thing, and how this information is supposed to get there.  It 
> seems mostly a Fedora/Systemd thing.  Benjamin is currently trying to 
> get that information into Ubuntu, and I'm also trying to get this into 
> Debian, when we see a viable approach without too many glitches.

The discussion on the bug report continued and I am posting what I wrote
in https://sourceware.org/bugzilla/show_bug.cgi?id=32003#c27 here for
wider visibility:

Taking all comments into account, here is my implementation proposals:

Encoding schema
===============

Option 1: Support percent-encoding of the JSON data. Percent-encoding is
widely used and supported (for example, Python provides
urllib.parse.unquote for decoding and urllib.parse.quote for encoding).
Example encoded JSON: '{%22foo%22:%22bar%22}' or
'%7B%22foo%22%3A%22bar%22%7D'

This option has the benefit of being easy to implement. Either the
encoded string can be read directly (I can spot package names and
version in there) or decoded using Python's urllib.parse.unquote or
online tools.

Option 2: Support percent-encoding and %[string] (where string refers to
the name in HTML's Named Character References) the JSON data. Example
encoded JSON: '{%[quot]foo%[quot]:%[quot]bar%[quot]}' or
'{%22foo%22:%22bar%22}'

This option allow people to use %[string] encoding in case they dislike
percent-encoding. The drawback is that it is more work to implement
since there must be a list of names. To make the code simpler, the list
of allowed names might be restricted to, e. g. quot, comma, lbrace,
rbrace and maybe add space.

These are the two options that I would be happy about to implement.
Supporting only %[string] would not satisfy me. Using base64 encoding
would make the string shorter, but would not be human readable. Quoted-
printable would be an alternative, but the problematic characters like
comma and quotes would not be encoded by quoted-printable.

Parameter usage
===============

Option A: Introduce a new --encoded-package-metadata parameter that
takes the encoded string.

Option B: Extend --package-metadata to always decode the given string.
As previously discussed, package names and version should not contain
percent characters. So this change should not break backward
compatibility.

Which of those proposals do you want to see implemented? My initial
implementation and patch v4 is option 1A.

-- 
Benjamin Drung
Debian & Ubuntu Developer