[PATCH v8 09/19] bfd: parse Object Attributes v2's section in input object files

[Matthieu Longo]

On 2025-08-08 14:03, Jan Beulich wrote:
> On 15.07.2025 13:39, Matthieu Longo wrote:
>> --- a/bfd/elf-attrs.c
>> +++ b/bfd/elf-attrs.c
>> @@ -835,7 +835,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;
>>   
>> @@ -954,7 +954,217 @@ bfd_elf_parse_attr_section_v1 (bfd *abfd, bfd_byte *p, bfd_byte *p_end)
>>       }
>>   }
>>   
>> -/* Parse an object attributes section.  */
>> +/* A helper struct for parsing, which returns the parsed object, the number of
>> +   bytes read, and whether or not an error occurred.  */
>> +typedef struct {
>> +  /* Was an error met during parsing.  */
>> +  bool err;
>> +  /* How many bytes were read ? (until error if an error occurred)  */
>> +  uint64_t read;
>> +  /* The parsed object.  */
>> +  void *object;
>> +} BufferReadOp_t ;
>> +
>> +#define READ_ULEB128(abfd, var, cursor, end, op)		\
>> +  do								\
>> +    {								\
>> +      bfd_byte *_begin = cursor;				\
>> +      (var) = _bfd_safe_read_leb128 (abfd, &cursor, false, end);\
> 
> It's pretty odd that you parenthesize "var" here (which must be an lvalue,
> and hence is more limited as to what can be passed in), but not "cursor",
> nor ...
> 

I surrounded all parameters with parenthesis.

>> +      op.read += cursor - _begin;				\
> 
> ... op.
> 
>> +    }								\
>> +  while (0)
>> +
>> +#define READ_NTBS(abfd, var, cursor, end, op)			\
>> +  do								\
>> +    {								\
>> +      (var) = strdup ((const char*) cursor);			\
> 
> Nit: Blank before * please.
> 

Fixed.
Note: I replaced strdup by xstrdup.

>> +      size_t read_ = strnlen (var, end - cursor) + 1;		\
>> +      op.read += read_;						\
> 
> Nit: There looks to be an issue with padding here.
> 

Fixed.

>> +      (cursor) += read_;					\
>> +    }								\
>> +  while (0)
>> +
>> +#define READ_UINT8(abfd, var, cursor, end, op)			\
>> +  do								\
>> +    {								\
>> +      (var) = bfd_get_8 (abfd, (cursor));			\
>> +      (cursor) += sizeof(uint8_t);				\
>> +      op.read += sizeof(uint8_t);				\
>> +    }								\
>> +  while (0)
> 
> None of these ever set op.err?
> 

No, bfd_get_8 does not return an error.

>> +/* Parse an object attribute (v2 only).  */
>> +static BufferReadOp_t
> 
> Returning structures is generally better avoided, imo.
> 

Why is it ? In this specific case, RVO will kick in and avoid copying 
the struct, unless that is only true for C++ ?

>> +oav2_parse_attr (bfd *abfd,
>> +		 bfd_byte *cursor,
>> +		 bfd_byte *const end,
> 
> Did you perhaps mean "const bfd_byte *end"?
> 

I actually meant "const bfd_byte *const end", I forgot a const.

>> +/* Parse a subsection (object attributes v2 only).  */
>> +static BufferReadOp_t
>> +oav2_parse_subsection (bfd *abfd,
>> +		       bfd_byte *cursor,
>> +		       const uint64_t max_read)
>> +{
>> +  BufferReadOp_t op = { .err = false, .read = 0, .object = NULL };
>> +
>> +  const uint32_t F_SUBSECTION_LEN = sizeof(uint32_t);
>> +  /* The minimum subsection length is 5: 4 bytes for the length itself, and 1
>> +     byte for an empty NUL-terminated string, and no vendor-data.  */
>> +  const uint32_t F_MIN_SUBSECTION_DATA_LEN = F_SUBSECTION_LEN + 1;
>> +
>> +  /* 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_SUBSECTION_LEN)
>> +    {
>> +      _bfd_error_handler (_("%pB: error: attributes subsection ends "
>> +			    "prematurely"), abfd);
>> +      bfd_set_error (bfd_error_malformed_archive);
>> +      return op;
>> +    }
>> +
>> +  uint32_t subsection_len = bfd_get_32 (abfd, cursor);
>> +  op.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);
>> +      bfd_set_error (bfd_error_malformed_archive);
>> +      op.err = true;
>> +      return op;
>> +    }
>> +  else if (subsection_len < F_MIN_SUBSECTION_DATA_LEN)
>> +    {
>> +      _bfd_error_handler (_("%pB: error: subsection length of %u is too small"),
>> +			  abfd, subsection_len);
>> +      bfd_set_error (bfd_error_malformed_archive);
>> +      op.err = true;
>> +      return op;
>> +    }
>> +
>> +  size_t subsection_name_len = strnlen ((char *) cursor, subsection_len) + 1;
>> +  if (subsection_name_len >= subsection_len)
>> +    {
>> +      _bfd_error_handler (_("%pB: error: subsection name seems corrupted "
>> +			    "(missing '\\0')"), abfd);
>> +      bfd_set_error (bfd_error_malformed_archive);
>> +      op.err = true;
>> +      return op;
>> +    }
>> +  /* 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.  */
>> +
>> +  unsigned char * const end = cursor + subsection_len - F_SUBSECTION_LEN;
> 
> More issues with * here and ...
> 

Fixed in the next revision.

>> +  BFD_ASSERT (cursor < end);
>> +
>> +  const char* subsection_name;
> 
> ... here. You really want to go through the entire series.
> 

Fixed in the next revision.

I fixed the ones I see but I don't have a tool to report them.
As I might have mentioned to you before, I use 
contrib/check_GNU_style.py from GCC's repo and it does not detect a bad 
positioning of *.

>> +  READ_NTBS (abfd, subsection_name, cursor, end, op);
>> +
>> +  uint8_t optional_raw;
>> +  READ_UINT8 (abfd, optional_raw, cursor, end, op);
>> +
>> +  if (optional_raw > 1)
>> +    {
>> +      _bfd_error_handler (_("%pB: error: optional value seems corrupted, got"
>> +			    " %u but only 0x0 (false) or 0x1 (true) are "
>> +			    "valid values."), abfd, optional_raw);
>> +      bfd_set_error (bfd_error_malformed_archive);
>> +      op.err = true;
>> +      free ((void*) subsection_name);
>> +      return op;
>> +    }
>> +
>> +  uint8_t attr_type_raw;
>> +  READ_UINT8 (abfd, attr_type_raw, cursor, end, op);
>> +  attr_type_raw = obj_attr_encoding_v2_from_u8 (attr_type_raw);
>> +  if (attr_type_raw > 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, attr_type_raw);
>> +      bfd_set_error (bfd_error_malformed_archive);
>> +      op.err = true;
>> +      free ((void*) subsection_name);
>> +      return op;
>> +    }
> 
> Much of this looks fairly familiar from patch 07 ...
> 

Yes, it is but there are slight differences in error handling, and also 
in the reading implementation. There was no uniform approach with OAv1 
between ld and readelf, so I continued on this trend because it would 
probably require more refactoring which I want to avoid.

>> @@ -996,7 +1206,12 @@ _bfd_elf_parse_attributes (bfd *abfd, Elf_Internal_Shdr * hdr)
>>     ++cursor;
>>   
>>     elf_obj_attr_version (abfd) = version;
>> -  bfd_elf_parse_attr_section_v1 (abfd, cursor, data + hdr->sh_size);
>> +  if (version == OBJ_ATTR_V1)
>> +    oav1_parse_section (abfd, cursor, data + hdr->sh_size);
>> +  else if (version == OBJ_ATTR_V2)
>> +    oav2_parse_section (abfd, hdr, cursor);
>> +  else
>> +    abort ();
> 
> Another opportunity to use switch()?

Fixed in the next revision.

> 
> Jan