Re: [PATCH] ld: add --package-metadata

[Fangrui Song <i@maskray.me>]

On 2022-05-15, luca.boccassi--- via Binutils wrote:
>From: Luca Boccassi <bluca@debian.org>
>
>Generate a .note.package FDO package metadata ELF note, following
>the spec: https://systemd.io/COREDUMP_PACKAGE_METADATA/
>
>If the jansson library is available at build time (and not explicitly
>disabled), link ld to it, and use it to validate that the input is
>correct JSON, to avoid writing garbage to the file.
>If no configure option is passed and jansson (or pkg-config) is not
>found, then it is just skipped (with a non-fatal warning). The
>configure option --enable-jansson can be used to explicitly enable it
>(error out when not found) or to explicitly disable it (don't even
>look for it). This allows bootstrappers (or others who are not
>interested) to seamlessly skip it without issues."
>---
>Now that this spec has shipped for all packages in Fedora 36, it seems
>like a good time to add a specific option for it to the default linker.
>Follows the same pattern of --build-id.

Both the "FDO" and the json dependency make me concerned -
if a linker script approach works quite well, why bother with a new
linker option with a very specific application?

--build-id is a bit different in that it adds a section whose content is
computed by the linker. I am not sure having --build-id implies
the necessity of --package-metadata.

>I've added an optional usage of libjansson to provide additional validation,
>so that it doesn't have to be reimplemented everywhere a thousand times over.
>For bootstrapping purposes the dep is optional and can be skipped, for example
>in Debian/Ubuntu it would be annotated with <!stage1> so that everything
>works out of the box.
>
> bfd/elf-bfd.h                        |   8 ++
> bfd/elf.c                            |   6 +-
> ld/Makefile.am                       |   6 +-
> ld/configure.ac                      |  35 ++++++++
> ld/emultempl/elf.em                  |   9 ++
> ld/ld.texi                           |   7 ++
> ld/ldelf.c                           | 121 ++++++++++++++++++++++++++-
> ld/ldelf.h                           |   2 +
> ld/lexsup.c                          |   2 +
> ld/testsuite/ld-elf/package-note.exp |  49 +++++++++++
> ld/testsuite/ld-elf/package-note.rd  |   6 ++
> 11 files changed, 244 insertions(+), 7 deletions(-)
> create mode 100644 ld/testsuite/ld-elf/package-note.exp
> create mode 100644 ld/testsuite/ld-elf/package-note.rd
>
>diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
>index c7c0a793b15..65bd1128263 100644
>--- a/bfd/elf-bfd.h
>+++ b/bfd/elf-bfd.h
>@@ -1918,6 +1918,14 @@ struct output_elf_obj_tdata
>     asection *sec;
>   } build_id;
>
>+  /* FDO_PACKAGING_METADATA note type info.  */
>+  struct
>+  {
>+    bool (*after_write_object_contents) (bfd *);
>+    const char *json;
>+    asection *sec;
>+  } package_metadata;
>+
>   /* Records the result of `get_program_header_size'.  */
>   bfd_size_type program_header_size;
>
>diff --git a/bfd/elf.c b/bfd/elf.c
>index c493aa5b172..558c630fd7b 100644
>--- a/bfd/elf.c
>+++ b/bfd/elf.c
>@@ -6779,8 +6779,10 @@ _bfd_elf_write_object_contents (bfd *abfd)
>     return false;
>
>   /* This is last since write_shdrs_and_ehdr can touch i_shdrp[0].  */
>-  if (t->o->build_id.after_write_object_contents != NULL)
>-    return (*t->o->build_id.after_write_object_contents) (abfd);
>+  if (t->o->build_id.after_write_object_contents != NULL && !(*t->o->build_id.after_write_object_contents) (abfd))
>+    return false;
>+  if (t->o->package_metadata.after_write_object_contents != NULL)
>+    return (*t->o->package_metadata.after_write_object_contents) (abfd);
>
>   return true;
> }
>diff --git a/ld/Makefile.am b/ld/Makefile.am
>index e53bef13bb8..a3cd7891840 100644
>--- a/ld/Makefile.am
>+++ b/ld/Makefile.am
>@@ -45,7 +45,7 @@ ELF_CLFAGS=-DELF_LIST_OPTIONS=@elf_list_options@ \
> 	   -DELF_PLT_UNWIND_LIST_OPTIONS=@elf_plt_unwind_list_options@
> WARN_CFLAGS = @WARN_CFLAGS@
> NO_WERROR = @NO_WERROR@
>-AM_CFLAGS = $(WARN_CFLAGS) $(ELF_CLFAGS)
>+AM_CFLAGS = $(WARN_CFLAGS) $(ELF_CLFAGS) $(JANSSON_CFLAGS)
>
> # We put the scripts in the directory $(scriptdir)/ldscripts.
> # We can't put the scripts in $(datadir) because the SEARCH_DIR
>@@ -964,8 +964,8 @@ ld_new_SOURCES = ldgram.y ldlex-wrapper.c lexsup.c ldlang.c mri.c ldctor.c ldmai
> 	ldwrite.c ldexp.c ldemul.c ldver.c ldmisc.c ldfile.c ldcref.c plugin.c \
> 	ldbuildid.c
> ld_new_DEPENDENCIES = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) \
>-		      $(BFDLIB) $(LIBCTF) $(LIBIBERTY) $(LIBINTL_DEP)
>-ld_new_LDADD = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) $(BFDLIB) $(LIBCTF) $(LIBIBERTY) $(LIBINTL) $(ZLIB)
>+		      $(BFDLIB) $(LIBCTF) $(LIBIBERTY) $(LIBINTL_DEP) $(JANSSON_LIBS)
>+ld_new_LDADD = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) $(BFDLIB) $(LIBCTF) $(LIBIBERTY) $(LIBINTL) $(ZLIB) $(JANSSON_LIBS)
>
> # Dependency tracking for the generated emulation files.
> EXTRA_ld_new_SOURCES += $(ALL_EMULATION_SOURCES) $(ALL_64_EMULATION_SOURCES)
>diff --git a/ld/configure.ac b/ld/configure.ac
>index 0b29e810dde..fdfb4d46344 100644
>--- a/ld/configure.ac
>+++ b/ld/configure.ac
>@@ -287,6 +287,41 @@ fi
> AM_CONDITIONAL(ENABLE_LIBCTF, test "${enable_libctf}" = yes)
> AC_SUBST(enable_libctf)
>
>+# If pkg-config and libjansson are available, use them, skip if not.
>+# Allow explicitly enabling (error if not found)/disabling (skip even if found).
>+AC_ARG_ENABLE([jansson],
>+  [AS_HELP_STRING([--enable-jansson],
>+    [enable jansson [default=auto]])],
>+  [enable_jansson=$enableval],
>+  [enable_jansson="auto"])
>+
>+if test "x$enable_jansson" != "xno"; then
>+  PKG_PROG_PKG_CONFIG
>+  AS_IF([test -n "$PKG_CONFIG"],
>+    [
>+      PKG_CHECK_MODULES(JANSSON, [jansson],
>+        [
>+          AC_DEFINE(HAVE_JANSSON, 1, [The jansson library is to be used])
>+          AC_SUBST([JANSSON_CFLAGS])
>+          AC_SUBST([JANSSON_LIBS])
>+        ],
>+        [
>+          if test "x$enable_jansson" = "xyes"; then
>+            AC_MSG_ERROR([Cannot find jansson library])
>+          else
>+            AC_MSG_WARN([Cannot find jansson library])
>+          fi
>+        ])
>+    ],
>+    [
>+      if test "x$enable_jansson" = "xyes"; then
>+        AC_MSG_ERROR([Cannot find pkg-config])
>+      else
>+        AC_MSG_WARN([Cannot find pkg-config])
>+      fi
>+    ])
>+fi
>+
> AM_BINUTILS_WARNINGS
>
> AM_LC_MESSAGES
>diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
>index c027559908c..c52484f35d3 100644
>--- a/ld/emultempl/elf.em
>+++ b/ld/emultempl/elf.em
>@@ -572,6 +572,7 @@ enum elf_options
>   OPTION_EXCLUDE_LIBS,
>   OPTION_HASH_STYLE,
>   OPTION_BUILD_ID,
>+  OPTION_PACKAGE_METADATA,
>   OPTION_AUDIT,
>   OPTION_COMPRESS_DEBUG
> };
>@@ -602,6 +603,7 @@ EOF
> fi
> fragment <<EOF
>     {"build-id", optional_argument, NULL, OPTION_BUILD_ID},
>+    {"package-metadata", optional_argument, NULL, OPTION_PACKAGE_METADATA},
>     {"compress-debug-sections", required_argument, NULL, OPTION_COMPRESS_DEBUG},
> EOF
> if test x"$GENERATE_SHLIB_SCRIPT" = xyes; then
>@@ -650,6 +652,13 @@ gld${EMULATION_NAME}_handle_option (int optc)
> 	ldelf_emit_note_gnu_build_id = xstrdup (optarg);
>       break;
>
>+    case OPTION_PACKAGE_METADATA:
>+      free ((char *) ldelf_emit_note_fdo_package_metadata);
>+      ldelf_emit_note_fdo_package_metadata = NULL;
>+      if (optarg != NULL && strlen(optarg) > 0)
>+	ldelf_emit_note_fdo_package_metadata = xstrdup (optarg);
>+      break;
>+
>     case OPTION_COMPRESS_DEBUG:
>       if (strcasecmp (optarg, "none") == 0)
> 	link_info.compress_debug = COMPRESS_DEBUG_NONE;
>diff --git a/ld/ld.texi b/ld/ld.texi
>index 8cad8478140..8406560397c 100644
>--- a/ld/ld.texi
>+++ b/ld/ld.texi
>@@ -2935,6 +2935,13 @@ 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 --package-metadata=@var{JSON}
>+@item --package-metadata=@var{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/COREDUMP_PACKAGE_METADATA/
> @end table
>
> @c man end
>diff --git a/ld/ldelf.c b/ld/ldelf.c
>index 4094640b3f7..161fb476121 100644
>--- a/ld/ldelf.c
>+++ b/ld/ldelf.c
>@@ -39,6 +39,9 @@
> #include <glob.h>
> #endif
> #include "ldelf.h"
>+#ifdef HAVE_JANSSON
>+#include <jansson.h>
>+#endif
>
> struct dt_needed
> {
>@@ -49,6 +52,9 @@ struct dt_needed
> /* Style of .note.gnu.build-id section.  */
> const char *ldelf_emit_note_gnu_build_id;
>
>+/* Content of .note.package section.  */
>+const char *ldelf_emit_note_fdo_package_metadata;
>+
> /* These variables are required to pass information back and forth
>    between after_open and check_needed and stat_needed and vercheck.  */
>
>@@ -1249,7 +1255,8 @@ ldelf_after_open (int use_libpath, int native, int is_linux, int is_freebsd,
> 	}
>     }
>
>-  if (ldelf_emit_note_gnu_build_id != NULL)
>+  if (ldelf_emit_note_gnu_build_id != NULL ||
>+      ldelf_emit_note_fdo_package_metadata != NULL)
>     {
>       /* Find an ELF input.  */
>       for (abfd = link_info.input_bfds;
>@@ -1262,11 +1269,18 @@ ldelf_after_open (int use_libpath, int native, int is_linux, int is_freebsd,
>       /* PR 10555: If there are no ELF input files do not try to
> 	 create a .note.gnu-build-id section.  */
>       if (abfd == NULL
>-	  || !ldelf_setup_build_id (abfd))
>+	  || (ldelf_emit_note_gnu_build_id != NULL && !ldelf_setup_build_id (abfd)))
> 	{
> 	  free ((char *) ldelf_emit_note_gnu_build_id);
> 	  ldelf_emit_note_gnu_build_id = NULL;
> 	}
>+
>+      if (abfd == NULL
>+	  || (ldelf_emit_note_fdo_package_metadata != NULL && !ldelf_setup_package_metadata (abfd)))
>+	{
>+	  free ((char *) ldelf_emit_note_fdo_package_metadata);
>+	  ldelf_emit_note_fdo_package_metadata = NULL;
>+	}
>     }
>
>   get_elf_backend_data (link_info.output_bfd)->setup_gnu_properties (&link_info);
>@@ -1501,6 +1515,109 @@ ldelf_setup_build_id (bfd *ibfd)
>   return false;
> }
>
>+static bool
>+write_package_metadata (bfd *abfd)
>+{
>+  struct elf_obj_tdata *t = elf_tdata (abfd);
>+  const char *json;
>+  asection *asec;
>+  Elf_Internal_Shdr *i_shdr;
>+  unsigned char *contents, *json_bits;
>+  bfd_size_type size;
>+  file_ptr position;
>+  Elf_External_Note *e_note;
>+
>+  json = t->o->package_metadata.json;
>+  asec = t->o->package_metadata.sec;
>+  if (bfd_is_abs_section (asec->output_section))
>+    {
>+      einfo (_("%P: warning: .note.package section discarded,"
>+	       " --package-metadata ignored\n"));
>+      return true;
>+    }
>+  i_shdr = &elf_section_data (asec->output_section)->this_hdr;
>+
>+  if (i_shdr->contents == NULL)
>+    {
>+      if (asec->contents == NULL)
>+	asec->contents = (unsigned char *) xmalloc (asec->size);
>+      contents = asec->contents;
>+    }
>+  else
>+    contents = i_shdr->contents + asec->output_offset;
>+
>+  e_note = (Elf_External_Note *) contents;
>+  size = offsetof (Elf_External_Note, name[sizeof "FDO"]);
>+  size = (size + 3) & -(bfd_size_type) 4;
>+  json_bits = contents + size;
>+  size = asec->size - size;
>+
>+  /* Clear the package metadata field.  */
>+  memset (json_bits, 0, size);
>+
>+  bfd_h_put_32 (abfd, sizeof "FDO", &e_note->namesz);
>+  bfd_h_put_32 (abfd, size, &e_note->descsz);
>+  bfd_h_put_32 (abfd, FDO_PACKAGING_METADATA, &e_note->type);
>+  memcpy (e_note->name, "FDO", sizeof "FDO");
>+  memcpy (json_bits, json, size);
>+
>+  position = i_shdr->sh_offset + asec->output_offset;
>+  size = asec->size;
>+  return (bfd_seek (abfd, position, SEEK_SET) == 0
>+	  && bfd_bwrite (contents, size, abfd) == size);
>+}
>+
>+/* Make .note.package section.
>+   https://systemd.io/COREDUMP_PACKAGE_METADATA/  */
>+
>+bool
>+ldelf_setup_package_metadata (bfd *ibfd)
>+{
>+  asection *s;
>+  bfd_size_type size;
>+  flagword flags;
>+
>+  if (!ldelf_emit_note_fdo_package_metadata || strlen(ldelf_emit_note_fdo_package_metadata) == 0)
>+    return false;
>+
>+#ifdef HAVE_JANSSON
>+  json_error_t json_error;
>+  json_t *json = json_loads(ldelf_emit_note_fdo_package_metadata, 0, &json_error);
>+  if (!json)
>+    {
>+      einfo (_("%P: warning: --package-metadata=%s does not contain valid "
>+	       "JSON, ignoring: %s\n"), ldelf_emit_note_fdo_package_metadata,
>+         json_error.text);
>+      return false;
>+    }
>+  else
>+    json_decref(json);
>+#endif
>+
>+  size = offsetof (Elf_External_Note, name[sizeof "FDO"]);
>+  size += strlen(ldelf_emit_note_fdo_package_metadata) + 1;
>+  size = (size + 3) & -(bfd_size_type) 4;
>+
>+  flags = (SEC_ALLOC | SEC_LOAD | SEC_IN_MEMORY
>+	   | SEC_LINKER_CREATED | SEC_READONLY | SEC_DATA);
>+  s = bfd_make_section_anyway_with_flags (ibfd, ".note.package",
>+					  flags);
>+  if (s != NULL && bfd_set_section_alignment (s, 2))
>+    {
>+      struct elf_obj_tdata *t = elf_tdata (link_info.output_bfd);
>+      t->o->package_metadata.after_write_object_contents = &write_package_metadata;
>+      t->o->package_metadata.json = ldelf_emit_note_fdo_package_metadata;
>+      t->o->package_metadata.sec = s;
>+      elf_section_type (s) = SHT_NOTE;
>+      s->size = size;
>+      return true;
>+    }
>+
>+  einfo (_("%P: warning: cannot create .note.package section,"
>+	   " --package-metadata ignored\n"));
>+  return false;
>+}
>+
> /* Look through an expression for an assignment statement.  */
>
> static void
>diff --git a/ld/ldelf.h b/ld/ldelf.h
>index efa8b45851c..a3ded3dd04c 100644
>--- a/ld/ldelf.h
>+++ b/ld/ldelf.h
>@@ -19,6 +19,7 @@
>    MA 02110-1301, USA.  */
>
> extern const char *ldelf_emit_note_gnu_build_id;
>+extern const char *ldelf_emit_note_fdo_package_metadata;
>
> extern void ldelf_after_parse (void);
> extern bool ldelf_load_symbols (lang_input_statement_type *);
>@@ -26,6 +27,7 @@ extern void ldelf_before_plugin_all_symbols_read (int, int, int, int,
> 						  int, const char *);
> extern void ldelf_after_open (int, int, int, int, int, const char *);
> extern bool ldelf_setup_build_id (bfd *);
>+extern bool ldelf_setup_package_metadata (bfd *);
> extern void ldelf_append_to_separated_string (char **, char *);
> extern void ldelf_before_allocation (char *, char *, const char *);
> extern bool ldelf_open_dynamic_archive
>diff --git a/ld/lexsup.c b/ld/lexsup.c
>index 78190472907..3aeba07c3a7 100644
>--- a/ld/lexsup.c
>+++ b/ld/lexsup.c
>@@ -2144,6 +2144,8 @@ elf_static_list_options (FILE *file)
>   fprintf (file, _("\
>   --build-id[=STYLE]          Generate build ID note\n"));
>   fprintf (file, _("\
>+  --package-metadata[=JSON]   Generate package metadata note\n"));
>+  fprintf (file, _("\
>   --compress-debug-sections=[none|zlib|zlib-gnu|zlib-gabi]\n\
>                               Compress DWARF debug sections using zlib\n"));
> #ifdef DEFAULT_FLAG_COMPRESS_DEBUG
>diff --git a/ld/testsuite/ld-elf/package-note.exp b/ld/testsuite/ld-elf/package-note.exp
>new file mode 100644
>index 00000000000..51a790b25de
>--- /dev/null
>+++ b/ld/testsuite/ld-elf/package-note.exp
>@@ -0,0 +1,49 @@
>+# Expect script for --package-note tests.
>+#   Copyright (C) 2022 Free Software Foundation, Inc.
>+#
>+# This file is part of the GNU Binutils.
>+#
>+# This program is free software; you can redistribute it and/or modify
>+# it under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# This program is distributed in the hope that it will be useful,
>+# but WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with this program; if not, write to the Free Software
>+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
>+# MA 02110-1301, USA.
>+#
>+
>+# Exclude non-ELF targets.
>+
>+if ![is_elf_format] {
>+    return
>+}
>+
>+if { [istarget frv-*-*] || [istarget lm32-*-*] } {
>+    return
>+}
>+
>+if { !([istarget *-*-linux*]
>+       || [istarget arm*-*-uclinuxfdpiceabi]
>+       || [istarget *-*-nacl*]
>+       || [istarget *-*-gnu*]) } then {
>+    return
>+}
>+
>+run_ld_link_tests [list \
>+    [list \
>+	"package-note.o" \
>+	"--package-metadata='{\"foo\":\"bar\"}'" \
>+	"" \
>+	"" \
>+	{start.s} \
>+	{{readelf {--notes} package-note.rd}} \
>+	"package-note.o" \
>+    ] \
>+]
>diff --git a/ld/testsuite/ld-elf/package-note.rd b/ld/testsuite/ld-elf/package-note.rd
>new file mode 100644
>index 00000000000..c65dd21cdac
>--- /dev/null
>+++ b/ld/testsuite/ld-elf/package-note.rd
>@@ -0,0 +1,6 @@
>+#...
>+Displaying notes found in: \.note\.package
>+  Owner                Data size 	Description
>+  FDO                  0x00000010	FDO_PACKAGING_METADATA
>+    Packaging Metadata: {"foo":"bar"}
>+#pass
>-- 
>2.35.1
>