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

[Jan Beulich]

On 06.11.2025 14:35, Matthieu Longo wrote:
> On 31/10/2025 10:25, Jan Beulich wrote:
>> On 01.09.2025 18:56, Matthieu Longo wrote:
>>> +  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 ?

Looks like so.

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

Is '_' a good character to use as separator? It's pretty commonly used in
identifiers. It's also unclear to me why subsection names would need
constraining to alphanumeric plus underscore. (Sorry, likely lacking some
background here.)

Jan