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

[Jan Beulich]

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

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

> +      size_t read_ = strnlen (var, end - cursor) + 1;		\
> +      op.read += read_;						\

Nit: There looks to be an issue with padding here.

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

> +/* Parse an object attribute (v2 only).  */
> +static BufferReadOp_t

Returning structures is generally better avoided, imo.

> +oav2_parse_attr (bfd *abfd,
> +		 bfd_byte *cursor,
> +		 bfd_byte *const end,

Did you perhaps mean "const bfd_byte *end"?

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

> +  BFD_ASSERT (cursor < end);
> +
> +  const char* subsection_name;

... here. You really want to go through the entire series.

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

> @@ -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()?

Jan