[PATCH v8 09/19] bfd: parse Object Attributes v2's section in input object files
Matthieu Longo
matthieu.longo@arm.com
Tue Aug 26 17:19:15 GMT 2025
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 ?
>>>> + (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.
>
I added checks for that in the new revision, and removed the macro.
Please have a look at the next revision.
>>>> +/* 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.
>
I removed BufferReadOp_t completely.
I don't think that the new code is more maintainable and readable. I
opted for using -1 to signal errors, which is not a great approach.
Please have a look at the code in the next revision, and let me know if
you know a better approach.
>>>> +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).
>
Ok, I removed it in the next revision.
>>>> +/* 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
More information about the Binutils
mailing list