diff mbox series

[2/2] gold: Support percent-encoded JSON in --package-metadata

Message ID 20241111100207.219703-2-benjamin.drung@canonical.com
State New
Headers
Series [1/2] ld: Support percent-encoded JSON in --package-metadata |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Benjamin Drung Nov. 11, 2024, 10:02 a.m. UTC 
  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 -fuse-ld=gold '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
/usr/bin/ld.gold: error: cannot open "os":"ubuntu"}: No such file or directory
/usr/bin/ld.gold: fatal error: error: --package-metadata={"type":"deb" does not contain valid JSON: '}' expected near end of file

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 support percent-encoded and %[string] encoded JSON data in the
`--package-metadata` linker flag. Percent-encoding is used because it is
a standard, simple to implement, and does take too many additional
characters. %[string] encoding is supported for having a more readable
encoding.

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>
---
 gold/options.cc            | 72 +++++++++++++++++++++++++++++++++
 gold/options.h             | 17 +++++++-
 gold/testsuite/Makefile.am | 16 +++++++-
 gold/testsuite/Makefile.in | 82 +++++++++++++++++++++++++++++++++-----
 4 files changed, 174 insertions(+), 13 deletions(-)
diff mbox series

Patch

diff --git a/gold/options.cc b/gold/options.cc
index 46f067fa72f..4f615d44e28 100644
--- a/gold/options.cc
+++ b/gold/options.cc
@@ -255,6 +255,78 @@  parse_string(const char* option_name, const char* arg, const char** retval)
   *retval = arg;
 }
 
+// Decode a percent and/or %[string] encoded string. Following %[string]
+// encodings are supported:
+//
+// %[comma] for ,
+// %[lbrace] for {
+// %[quot] for "
+// %[rbrace] for }
+// %[space] for ' '
+//
+// The percent decoding behaves the same as Python's urllib.parse.unquote.
+void
+parse_optional_encoded_string(const char* option_name, const char* arg, const char** retval)
+{
+  hex_init();
+  size_t arg_len = strlen (arg);
+  char *package_metadata = (char *)malloc (arg_len + 1);
+  if (package_metadata == NULL)
+    gold_fatal(_("%s: malloc failed"), option_name);
+
+  const char *src = arg;
+  char *dst = package_metadata;
+  while (*src != '\0')
+    {
+      char c = *src++;
+      if (c == '%')
+	{
+	  char next1 = *src;
+	  if (next1 != '\0' && hex_p(next1))
+	    {
+	      char next2 = *(src + 1);
+	      if (next2 != '\0' && hex_p(next2))
+		{
+		  c = (char)((hex_value(next1) << 4) + hex_value(next2));
+		  src += 2;
+		}
+	    }
+	  else if (next1 == '[')
+	    {
+	      if (strncmp(src + 1, "comma]", 6) == 0)
+		{
+		  c = ',';
+		  src += 7;
+		}
+	      else if (strncmp(src + 1, "lbrace]", 7) == 0)
+		{
+		  c = '{';
+		  src += 8;
+		}
+	      else if (strncmp(src + 1, "quot]", 5) == 0)
+		{
+		  c = '"';
+		  src += 6;
+		}
+	      else if (strncmp(src + 1, "rbrace]", 7) == 0)
+		{
+		  c = '}';
+		  src += 8;
+		}
+	      else if (strncmp(src + 1, "space]", 6) == 0)
+		{
+		  c = ' ';
+		  src += 7;
+		}
+	    }
+	}
+      *dst++ = c;
+    }
+  *dst = '\0';
+
+  *retval = package_metadata;
+}
+
 void
 parse_optional_string(const char*, const char* arg, const char** retval)
 {
diff --git a/gold/options.h b/gold/options.h
index 446e8d42614..78766c1fc6b 100644
--- a/gold/options.h
+++ b/gold/options.h
@@ -108,6 +108,10 @@  parse_percent(const char* option_name, const char* arg, double* retval);
 extern void
 parse_string(const char* option_name, const char* arg, const char** retval);
 
+extern void
+parse_optional_encoded_string(const char* option_name, const char* arg,
+			      const char** retval);
+
 extern void
 parse_optional_string(const char* option_name, const char* arg,
 		      const char** retval);
@@ -589,6 +593,17 @@  struct Struct_special : public Struct_var
   };                                                                    \
   Struct_##varname__ varname__##_initializer_
 
+// An option that takes an optional string argument.  If the option is
+// used with no argument, the value will be the default, and
+// user_set_via_option will be true.
+#define DEFINE_optional_encoded_string(varname__, dashes__,		\
+				       shortname__, default_value__,	\
+				       helpstring__, helparg__)		\
+  DEFINE_var(varname__, dashes__, shortname__, default_value__,		\
+	     default_value__, helpstring__, helparg__, true,		\
+	     const char*, const char*,					\
+	     options::parse_optional_encoded_string, false)
+
 // An option that takes an optional string argument.  If the option is
 // used with no argument, the value will be the default, and
 // user_set_via_option will be true.
@@ -1106,7 +1121,7 @@  class General_options
   DEFINE_bool(p, options::ONE_DASH, 'p', false,
 	      N_("Ignored for ARM compatibility"), NULL);
 
-  DEFINE_optional_string(package_metadata, options::TWO_DASHES, '\0', NULL,
+  DEFINE_optional_encoded_string(package_metadata, options::TWO_DASHES, '\0', NULL,
 			 N_("Generate package metadata note"),
 			 N_("[=JSON]"));
 
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 8f158ba20cc..d7f496c75c1 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -4470,9 +4470,21 @@  retain_2.o: retain_2.s
 
 endif DEFAULT_TARGET_X86_64
 
-check_PROGRAMS += package_metadata_test
+check_PROGRAMS += package_metadata_test1 package_metadata_test1b package_metadata_test1c package_metadata_test2 package_metadata_test2b
 package_metadata_test.o: package_metadata_main.c
 	$(COMPILE) -c -o $@ $<
-package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
+package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
 	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
 	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
+package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
index 357dec0d4f9..6cf619fd974 100644
--- a/gold/testsuite/Makefile.in
+++ b/gold/testsuite/Makefile.in
@@ -110,7 +110,11 @@  check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
 	$(am__EXEEXT_40) $(am__EXEEXT_41) $(am__EXEEXT_42) \
 	$(am__EXEEXT_43) $(am__EXEEXT_44) $(am__EXEEXT_45) \
 	$(am__EXEEXT_46) $(am__EXEEXT_47) $(am__EXEEXT_48) \
-	package_metadata_test$(EXEEXT)
+	package_metadata_test1$(EXEEXT) \
+	package_metadata_test1b$(EXEEXT) \
+	package_metadata_test1c$(EXEEXT) \
+	package_metadata_test2$(EXEEXT) \
+	package_metadata_test2b$(EXEEXT)
 @NATIVE_OR_CROSS_LINKER_TRUE@am__append_1 = object_unittest \
 @NATIVE_OR_CROSS_LINKER_TRUE@	binary_unittest leb128_unittest \
 @NATIVE_OR_CROSS_LINKER_TRUE@	overflow_unittest
@@ -1799,9 +1803,21 @@  overflow_unittest_OBJECTS = $(am_overflow_unittest_OBJECTS)
 @NATIVE_OR_CROSS_LINKER_TRUE@	$(am__DEPENDENCIES_1)
 overflow_unittest_LINK = $(CXXLD) $(AM_CXXFLAGS) $(CXXFLAGS) \
 	$(overflow_unittest_LDFLAGS) $(LDFLAGS) -o $@
-package_metadata_test_SOURCES = package_metadata_test.c
-package_metadata_test_OBJECTS = package_metadata_test.$(OBJEXT)
-package_metadata_test_LDADD = $(LDADD)
+package_metadata_test1_SOURCES = package_metadata_test1.c
+package_metadata_test1_OBJECTS = package_metadata_test1.$(OBJEXT)
+package_metadata_test1_LDADD = $(LDADD)
+package_metadata_test1b_SOURCES = package_metadata_test1b.c
+package_metadata_test1b_OBJECTS = package_metadata_test1b.$(OBJEXT)
+package_metadata_test1b_LDADD = $(LDADD)
+package_metadata_test1c_SOURCES = package_metadata_test1c.c
+package_metadata_test1c_OBJECTS = package_metadata_test1c.$(OBJEXT)
+package_metadata_test1c_LDADD = $(LDADD)
+package_metadata_test2_SOURCES = package_metadata_test2.c
+package_metadata_test2_OBJECTS = package_metadata_test2.$(OBJEXT)
+package_metadata_test2_LDADD = $(LDADD)
+package_metadata_test2b_SOURCES = package_metadata_test2b.c
+package_metadata_test2b_OBJECTS = package_metadata_test2b.$(OBJEXT)
+package_metadata_test2b_LDADD = $(LDADD)
 permission_test_SOURCES = permission_test.c
 permission_test_OBJECTS = permission_test.$(OBJEXT)
 permission_test_LDADD = $(LDADD)
@@ -2355,7 +2371,9 @@  SOURCES = $(libgoldtest_a_SOURCES) $(aarch64_pr23870_SOURCES) \
 	$(large_symbol_alignment_SOURCES) $(leb128_unittest_SOURCES) \
 	local_labels_test.c many_sections_r_test.c \
 	$(many_sections_test_SOURCES) $(object_unittest_SOURCES) \
-	$(overflow_unittest_SOURCES) package_metadata_test.c \
+	$(overflow_unittest_SOURCES) package_metadata_test1.c \
+	package_metadata_test1b.c package_metadata_test1c.c \
+	package_metadata_test2.c package_metadata_test2b.c \
 	permission_test.c $(pie_copyrelocs_test_SOURCES) \
 	plugin_test_1.c plugin_test_10.c plugin_test_11.c \
 	plugin_test_12.c plugin_test_2.c plugin_test_3.c \
@@ -4869,7 +4887,11 @@  distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/many_sections_test.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/object_unittest.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/overflow_unittest.Po@am__quote@
-@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1b.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1c.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2b.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/permission_test.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pie_copyrelocs_test-pie_copyrelocs_test.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/plugin_test_1.Po@am__quote@
@@ -7854,9 +7876,37 @@  aarch64_pr23870.log: aarch64_pr23870$(EXEEXT)
 	--log-file $$b.log --trs-file $$b.trs \
 	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
 	"$$tst" $(AM_TESTS_FD_REDIRECT)
-package_metadata_test.log: package_metadata_test$(EXEEXT)
-	@p='package_metadata_test$(EXEEXT)'; \
-	b='package_metadata_test'; \
+package_metadata_test1.log: package_metadata_test1$(EXEEXT)
+	@p='package_metadata_test1$(EXEEXT)'; \
+	b='package_metadata_test1'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test1b.log: package_metadata_test1b$(EXEEXT)
+	@p='package_metadata_test1b$(EXEEXT)'; \
+	b='package_metadata_test1b'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test1c.log: package_metadata_test1c$(EXEEXT)
+	@p='package_metadata_test1c$(EXEEXT)'; \
+	b='package_metadata_test1c'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test2.log: package_metadata_test2$(EXEEXT)
+	@p='package_metadata_test2$(EXEEXT)'; \
+	b='package_metadata_test2'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test2b.log: package_metadata_test2b$(EXEEXT)
+	@p='package_metadata_test2b$(EXEEXT)'; \
+	b='package_metadata_test2b'; \
 	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
 	--log-file $$b.log --trs-file $$b.trs \
 	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
@@ -10521,9 +10571,21 @@  uninstall-am:
 @DEFAULT_TARGET_X86_64_TRUE@	$(TEST_AS) -o $@ $<
 package_metadata_test.o: package_metadata_main.c
 	$(COMPILE) -c -o $@ $<
-package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
+package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
 	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
 	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
+package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
 
 # Tell versions [3.59,3.63) of GNU make to not export all variables.
 # Otherwise a system limit (for SysV at least) may be exceeded.