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

[Jan Beulich]

On 15.08.2025 13:25, Matthieu Longo wrote:
> 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.

I hope you don't mean this literally. In the function invocation above,
"end" doesn't need parenthesizing, just to give an example.

>>> +      (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.

That wasn't my point though. You may overrun "end", afaict.

>>> +/* 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++ ?

I haven't come across RVO as an acronym, and if it's Return Value
Optimization that could still mean about anything. In any event, returning
structures by value on various architectures has "interesting" ABIs, which
can easily be quirky. That's perhaps less of an issue when the function is
static, but I still would prefer not to set undesirable precedents.

>>> +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.

The latter const isn't very useful, though (and its presence there is then
also inconsistent with none being there for "abfd", nor hardly anywhere
else in the code base).

>>> +/* 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 *.

Right. I don't use any tool. It's a matter of getting used to the style,
plus sometimes looking at one's own patches before having reviewers do so.
(It may also be possible to use e.g. grep to do a fair amount of such
checking, without ending up with too many false positives / negatives.)

Jan