ping for [PATCH] gold: Support percent-encoded JSON in --package-metadata
Sam James
sam@gentoo.org
Tue Dec 17 12:27:25 GMT 2024
Benjamin Drung <benjamin.drung@canonical.com> writes:
> Friendly ping since I haven't got a response since submission three
> weeks ago.
I recommend CCing the gold maintainers.
>
> On Wed, 2024-11-27 at 10:24 +0000, 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 -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.
>>
>> Following the same format as the implementation in ld:
>> b0cc81e87087bb8a6b12dc1e4fd7f2591927977b
>>
>> 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 --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.
More information about the Binutils
mailing list