[PATCH v9 09/19] bfd: parse Object Attributes v2's section in input object files
Matthieu Longo
matthieu.longo@arm.com
Thu Nov 6 13:35:19 GMT 2025
On 31/10/2025 10:25, Jan Beulich wrote:
> On 01.09.2025 18:56, Matthieu Longo wrote:
>> This patch adds to BFD the capability of parsing the Object Attributes v2
>> (OAv2) stored into the section defined by the backend (i.e. .ARM.attributes
>> for AArch64).
>>
>> An OAv2 section is organized with the following syntactic structure:
>> <format-version: uint8>
>> [ <subsection-length: uint32>
>> <subsection-name: NTBS>
>> <optionality: uint8>
>> <encoding: uint8>
>> <attribute>*
>> ]*
>>
>> Once parsing is completed, OAv2 are stored in a list of subsections in
>> elf_obj_tdata.
>>
>> Regarding ULEB128 parsing, the implementation of _bfd_safe_read_leb128
>> has known shortcomings. In particular, it may accept malformed ULEB128
>> values if the 8th bit is set while meeting the end condition "data < end".
>> The error handling could be improved, but this function is old and used
>> in more than 50 places. The same issue applies to _bfd_read_unsigned_leb128
>> and _bfd_read_signed_leb128, which likewise do not report errors in
>> this case. Since this is long-standing behavior and fixing it would
>> require broader changes outside the scope of this patch series, the
>> issue is acknowledged but not addressed here, and is considered
>> acceptable as this patch does not worsen the situation.
>> ---
>> bfd/elf-attrs.c | 254 +++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 251 insertions(+), 3 deletions(-)
>
> The way the helper functions are (naturally) organized, I decided to look at
> this bottom-up (but of course top-down within each function). Reading the
> comments in this order may therefore make a little more sense here and there.
>
>> --- a/bfd/elf-attrs.c
>> +++ b/bfd/elf-attrs.c
>> @@ -843,7 +843,7 @@ _bfd_elf_obj_attrs_arg_type (bfd *abfd,
>> }
>>
>> static void
>> -bfd_elf_parse_attr_section_v1 (bfd *abfd, bfd_byte *p, bfd_byte *p_end)
>> +oav1_parse_section (bfd *abfd, bfd_byte *p, bfd_byte *p_end)
>> {
>> const char *std_sec = get_elf_backend_data (abfd)->obj_attrs_vendor;
>>
>> @@ -962,7 +962,245 @@ bfd_elf_parse_attr_section_v1 (bfd *abfd, bfd_byte *p, bfd_byte *p_end)
>> }
>> }
>>
>> -/* Parse an object attributes section. */
>> +#define READ_ULEB128(abfd, var, cursor, end, total_read) \
>> + do \
>> + { \
>> + bfd_byte *begin = (cursor); \
>> + (var) = _bfd_safe_read_leb128 (abfd, &(cursor), false, end); \
>> + (total_read) += (cursor) - begin; \
>> + } \
>> + while (0)
>> +
>> +static int
>> +read_ntbs (bfd *abfd,
>> + const bfd_byte *cursor,
>> + const bfd_byte *end,
>> + const char **s)
>> +{
>> + *s = NULL;
>> + const size_t MAX_STR_LEN = end - cursor; /* Including \0. */
>> + const size_t s_len = strnlen ((char *) cursor, MAX_STR_LEN);
>
> Please don't cast away const-ness unless you really need to. See ...
>
Fixed.
>> + if (s_len == MAX_STR_LEN)
>> + {
>> + bfd_set_error (bfd_error_malformed_archive);
>> + _bfd_error_handler (_("%pB: error: NTBS value seems corrupted "
>> + "(missing '\\0')"), abfd);
>> + return -1;
>> + }
>> + *s = xstrdup ((const char *) cursor);
>
> ... e.g. here.
>
> This latter cast could actually be avoided: As you know the length, you could
> easily use xmemdup() instead. That'll also be cheaper, as the length won't
> need determining a 2nd time then.
>
Replaced by using xmemdup().
>> + return s_len + 1;
>> +}
>> +
>> +/* Parse an object attribute (v2 only). */
>> +static int
>> +oav2_parse_attr (bfd *abfd,
>> + bfd_byte *cursor,
>> + const bfd_byte *end,
>> + obj_attr_encoding_v2 attr_type,
>> + obj_attr_v2 **attr)
>> +{
>> + *attr = NULL;
>> + size_t total_read = 0;
>> +
>> + obj_attr_tag_t attr_tag;
>> + READ_ULEB128 (abfd, attr_tag, cursor, end, total_read);
>> +
>> + union obj_attr_value_v2 attr_val;
>> + switch (attr_type)
>> + {
>> + case OA_ENC_NTBS:
>> + {
>> + int read = read_ntbs (abfd, cursor, end, &attr_val.string_val);
>> + if (read <= 0)
>> + return -1;
>> + total_read += read;
>> + }
>> + break;
>> + case OA_ENC_ULEB128:
>> + READ_ULEB128 (abfd, attr_val.uint_val, cursor, end, total_read);
>> + break;
>> + default:
>> + abort ();
>> + }
>> +
>> + *attr = _bfd_elf_obj_attr_v2_init (attr_tag, attr_val);
>> + return total_read;
>> +}
>> +
>> +/* Parse a subsection (object attributes v2 only). */
>> +static int
>> +oav2_parse_subsection (bfd *abfd,
>> + bfd_byte *cursor,
>> + const uint64_t max_read,
>> + obj_attr_subsection_v2 **subsec)
>> +{
>> + *subsec = NULL;
>> + size_t total_read = 0;
>> +
>> + const char *subsection_name = NULL;
>
> I'm all for pointer-to-const where it's helpful. Here it doesn't look to be; its
> main (bad) effect is the need to cast away the const when calling free(). For the
> purpose of the function it also doesn't matter - it is here where the string is
> read (and hence being modified anyway).
>
Removed const here, so the cast on the free() too.
>> + const uint32_t F_SUBSECTION_LEN = sizeof(uint32_t);
>> + const uint32_t F_SUBSECTION_COMPREHENSION = sizeof(uint8_t);
>> + const uint32_t F_SUBSECTION_ENCODING = sizeof(uint8_t);
>> + /* The minimum subsection length is 7: 4 bytes for the length itself, and 1
>> + byte for an empty NUL-terminated string, 1 byte for the comprehension,
>> + 1 byte for the encoding, and no vendor-data. */
>> + const uint32_t F_MIN_SUBSECTION_DATA_LEN
>> + = F_SUBSECTION_LEN + 1 /* for '\0' */
>> + + F_SUBSECTION_COMPREHENSION + F_SUBSECTION_ENCODING;
>> +
>> + /* Similar to the issues reported in PR 17531, we need to check all the sizes
>> + and offsets as we parse the section. */
>> + if (max_read < F_MIN_SUBSECTION_DATA_LEN)
>> + {
>> + _bfd_error_handler (_("%pB: error: attributes subsection ends "
>> + "prematurely"), abfd);
>> + goto error;
>> + }
>> +
>> + const uint32_t subsection_len = bfd_get_32 (abfd, cursor);
>> + total_read += F_SUBSECTION_LEN;
>> + cursor += F_SUBSECTION_LEN;
>> + if (subsection_len > max_read)
>> + {
>> + _bfd_error_handler (_("%pB: error: bad subsection length (%u > max=%lu)"),
>> + abfd, subsection_len, max_read);
>> + goto error;
>> + }
>> + else if (subsection_len < F_MIN_SUBSECTION_DATA_LEN)
>> + {
>> + _bfd_error_handler (_("%pB: error: subsection length of %u is too small"),
>> + abfd, subsection_len);
>> + goto error;
>> + }
>> +
>> + const size_t MAX_SUBSECTION_NAME_LEN
>> + = subsection_len - F_SUBSECTION_LEN
>> + - F_SUBSECTION_COMPREHENSION - F_SUBSECTION_ENCODING;
>
> Other all-capitals identifiers are kind-of #define-s of build-time constants.
> This one isn't a constant, though, so the use of such an identifier feels a
> little misleading.
>
Fixed in the next revision.
>> + const size_t subsection_name_len
>> + = strnlen ((char *) cursor, MAX_SUBSECTION_NAME_LEN);
>
> I think I mentioned earlier that casts would better be used sparingly. Here we
> could get away without, if we used memchr() instead. Thoughts?
>
Is it what you had in mind ?
diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
index f17fe1dc541..57e61c2fb46 100644
--- a/bfd/elf-attrs.c
+++ b/bfd/elf-attrs.c
@@ -2601,6 +2601,7 @@ oav2_parse_subsection (bfd *abfd,
}
const uint32_t subsection_len = bfd_get_32 (abfd, cursor);
+ const bfd_byte *const end = cursor + subsection_len;
total_read += F_SUBSECTION_LEN;
cursor += F_SUBSECTION_LEN;
if (subsection_len > max_read)
@@ -2619,20 +2620,22 @@ oav2_parse_subsection (bfd *abfd,
const size_t MAX_SUBSECTION_NAME_LEN
= subsection_len - F_SUBSECTION_LEN
- F_SUBSECTION_COMPREHENSION - F_SUBSECTION_ENCODING;
- const size_t subsection_name_len
- = strnlen ((char *) cursor, MAX_SUBSECTION_NAME_LEN);
- if (subsection_name_len >= MAX_SUBSECTION_NAME_LEN)
+ const bfd_byte *subsection_name_end
+ = memchr (cursor, '\0', MAX_SUBSECTION_NAME_LEN);
+ if (subsection_name_end == NULL)
{
_bfd_error_handler (_("%pB: error: subsection name seems corrupted "
"(missing '\\0')"), abfd);
goto error;
}
+ else
+ /* Move the end pointer after '\0'. */
+ ++subsection_name_end;
+
/* Note: if the length of the subsection name is 0 (i.e. the string
is '\0'),
it is still considered a valid name, even if it is not particularly
useful. */
- const bfd_byte *const end = cursor - F_SUBSECTION_LEN + subsection_len;
-
/* Note: at this stage,
1. the length of the subsection name is validated, as the
presence of '\0'
at the end of the string, so no risk of buffer overrun.
@@ -2641,8 +2644,7 @@ oav2_parse_subsection (bfd *abfd,
/* Note: read_ntbs() assigns a dynamically allocated string to
subsection_name. Either the string has to be freed in case of
errors,
or its ownership must be transferred. */
- int read = read_ntbs (abfd, cursor, cursor + subsection_name_len + 1,
- &subsection_name);
+ int read = read_ntbs (abfd, cursor, subsection_name_end,
&subsection_name);
total_read += read;
cursor += read;
}
>> + if (subsection_name_len >= MAX_SUBSECTION_NAME_LEN)
>> + {
>> + _bfd_error_handler (_("%pB: error: subsection name seems corrupted "
>> + "(missing '\\0')"), abfd);
>> + goto error;
>> + }
>> + /* Note: if the length of the subsection name is 0 (i.e. the string is '\0'),
>> + it is still considered a valid name, even if it is not particularly
>> + useful. */
>> +
>> + const bfd_byte *const end = cursor - F_SUBSECTION_LEN + subsection_len;
>
> The expression could be simplified if this was moved up a little, and the
> earlier update of cursor also moved down some.
>
See diff in previous answer.
>> + /* Note: at this stage,
>> + 1. the length of the subsection name is validated, as the presence of '\0'
>> + at the end of the string, so no risk of buffer overrun.
>> + 2. the data for comprehension and encoding can also safely be read. */
>> + {
>> + int read = read_ntbs (abfd, cursor, cursor + subsection_name_len + 1,
>> + &subsection_name);
>> + total_read += read;
>> + cursor += read;
>> + }
>> +
>> + uint8_t comprehension_raw = bfd_get_8 (abfd, cursor);
>> + ++cursor;
>> + ++total_read;
>> +
>> + /* Comprehension is supposed to be a boolean, so any value greater than 1 is
>> + considered invalid. */
>> + if (comprehension_raw > 1)
>> + {
>> + _bfd_error_handler (_("%pB: error: '%s' seems corrupted, got %u but only "
>> + "0x0 ('%s') or 0x1 ('%s') are valid values."),
>
> Are the 0x here of any real use?
>
No, I removed them.
> Also, throughout the series (as this looks to be repeating a lot, albeit you're
> also not consistent there): Diagnostics should normally not end in a full stop.
>
Removed in others messages of this patch.
>> + abfd, "comprehension", comprehension_raw,
>> + "required", "optional");
>> + goto error;
>> + }
>> +
>> + uint8_t value_encoding_raw = bfd_get_8 (abfd, cursor);
>> + ++cursor;
>> + ++total_read;
>> +
>> + /* Encoding cannot be greater than OA_ENC_MAX, otherwise it means that either
>> + there is a new encoding that was introduced in the spec, and this
>> + implementation in binutils is older, and not aware of it so does not
>> + support it; or the stored value for encoding is garbage. */
>> + enum obj_attr_encoding_v2 value_encoding
>> + = obj_attr_encoding_v2_from_u8 (value_encoding_raw);
>> + if (value_encoding > OA_ENC_MAX)
>> + {
>> + _bfd_error_handler (_("%pB: error: attribute type seems corrupted, got"
>> + " %u but only 0x0 (ULEB128) or 0x1 (NTBS) are "
>> + "valid types."), abfd, value_encoding);
>
> Same remarks here.
>
Fixed.
>> + goto error;
>> + }
>> +
>> + const char *vendor_name = get_elf_backend_data (abfd)->obj_attrs_vendor;
>> + obj_attr_subsection_scope_v2 scope
>> + = (strncmp (subsection_name, vendor_name, strlen (vendor_name)) == 0
>> + ? OA_SUBSEC_PUBLIC
>> + : OA_SUBSEC_PRIVATE);
>
> What is this following from? I'm in particular puzzled that there's no form of
> separator required between the part potentially matching the sub-section name
> and the rest. This way a sub-section XYZ would be public for vendor XY, which
> may or may not be what is intended. Personally I would have expected
> separation by e.g. some non-alphanumeric character.
>
I discussed that with the person writing the specs, and he does not have
any objection to add a separator between the prefix and the core.
So, in the next revision, the parser for a subsection directive only
accept [a-zA-Z][a-zA-Z0-9_]* for the subsection name, and '_' is
considered a separator.
I moved this code into a helper that I can use between gas, and bfd.
Readelf will continue with its own version.
Here below is the final version of the helper at the top of the branch
in bfd:
/* Identify the scope of a subsection from its name. */
obj_attr_subsection_scope_v2
bfd_elf_obj_attr_subsection_v2_scope (bfd *abfd, const char *subsec_name)
{
const char *vendor_name = get_elf_backend_data (abfd)->obj_attrs_vendor;
obj_attr_subsection_scope_v2 scope = OA_SUBSEC_PRIVATE;
size_t vendor_name_len = strlen (vendor_name);
if ((strncmp (subsec_name, vendor_name, vendor_name_len) == 0
&& subsec_name[vendor_name_len] == '_')
|| (strncmp (subsec_name, "gnu_", 4) == 0
&& !gnu_testing_namespace (subsec_name)))
scope = OA_SUBSEC_PUBLIC;
return scope;
}
>> + *subsec = _bfd_elf_obj_attr_subsection_v2_init
>> + (subsection_name, scope, comprehension_raw, value_encoding);
>> +
>> + /* A subsection can be empty, so 'cursor' can be equal to 'end' here. */
>> + bool err = false;
>> + while (!err && cursor < end)
>> + {
>> + obj_attr_v2 *attr;
>> + int read = oav2_parse_attr (abfd, cursor, end, value_encoding, &attr);
>> + LINKED_LIST_APPEND(obj_attr_v2) (*subsec, attr);
>
> If you got back an error, you have nothing to append (you'd append NULL, which
> I don't expect will be liked by the list machinery).
>
Thanks for catching that.
Fixed.
>> + total_read += read;
>> + err |= (read < 0);
>> + cursor += read;
>> + }
>> +
>> + if (err)
>> + {
>> + _bfd_elf_obj_attr_subsection_v2_free (*subsec);
>> + *subsec = NULL;
>> + return -1;
>> + }
>> +
>> + BFD_ASSERT (cursor == end);
>> + return total_read;
>> +
>> + error:
>> + bfd_set_error (bfd_error_malformed_archive);
>> + if (subsection_name)
>> + free ((void *) subsection_name);
>> + return -1;
>> +}
>> +
>> +/* Parse the list of subsections (object attributes v2 only). */
>> +static void
>> +oav2_parse_section (bfd *abfd,
>> + Elf_Internal_Shdr *hdr,
>> + bfd_byte *cursor)
>
> As previously requested: Pointer-to-const please wherever possible. At least
> the latter two parameters look to qualify. Hmm, or maybe not for cursor, due
> to _bfd_safe_read_leb128() wanting it non-const.
>
Constified hdr.
>> +{
>> + obj_attr_subsection_list *subsecs = &elf_obj_attr_subsections (abfd);
>> + int read = 0;
>> + for (uint64_t remaining = hdr->sh_size - 1; /* Already read 'A'. */
>> + remaining > 0;
>> + remaining -= read, cursor += read)
>> + {
>> + obj_attr_subsection_v2 *subsec = NULL;
>> + read = oav2_parse_subsection (abfd, cursor, remaining, &subsec);
>> + if (read < 0)
>
> "read" being plain int, it looks to be implied that a subsection can't
> exceed 2Gb in size? (There's no explicit check anywhere that I could spot.
> And yes, such a big section might indeed be insane, but I assume you know
> what e.g. fuzzers do.)
>
Replaced by ssize_t. I also changed the return type of
oav2_parse_subsection().
>> + {
>> + _bfd_error_handler (_("%pB: error: could not parse subsection at "
>> + "offset %lx"),
>> + abfd, hdr->sh_size - remaining);
>
> As also previously indicated: You can't use %lx with uint64_t-type variables.
> It needs to remain possible to build binutils on 32-bit hosts.
>
> Jan
Fixed in the next revision.
Matthieu
More information about the Binutils
mailing list