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

[Matthieu Longo]

On 2025-08-27 08:31, Jan Beulich wrote:
> On 26.08.2025 19:19, Matthieu Longo wrote:
>> On 2025-08-15 13:38, Jan Beulich wrote:
>>> 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.
>>
>> I haven't surrounded "abfd" and "end".
>>
>> Regarding this macro, it is the only one I kept in the next revision,
>> the reason being the possibly different type for "var".
>>
>> The current implementation of _bfd_safe_read_leb128 does not return an
>> error if the 8th bit is set while meeting the end condition "data <
>> end". The error checking could be improved, but this is old code that is
>> used in more than 50 places. Same comment can be done for
>> _bfd_read_unsigned_leb128 and _bfd_read_signed_leb128 that don't return
>> any error.
>>
>> Even if the current implementation is not perfect and might accept
>> malformed ULEB128, it is not worse than the current situation.
>> Please, could we consider fixing the signature of those functions to
>> return an error out of scope of this patch series ?
> 
> Hmm, I'm not a fan of seeing new code go in with known shortcomings. If
> this is to be an option, such shortcomings would at the very least need
> to be prominently named, so people are aware that they've been knowingly
> left there.
> 
> Jan

I will add a note in the commit description.

Matthieu