Re: [PATCH] bfd: make _bfd_section_size_insane part of the public API

[Andrew Burgess <aburgess@redhat.com>]

Jan Beulich <jbeulich@suse.com> writes:

> On 06.12.2023 17:15, Andrew Burgess wrote:
>> If a BFD user is making use of a function like
>> bfd_get_section_contents to read a section into a pre-allocated
>> buffer, then that BFD user might also want to make use of
>> _bfd_section_size_insane prior to allocating the buffer they intend to
>> use in order to validate that the buffer size that plan to allocate is
>> sane.
>> 
>> This commit makes _bfd_section_size_insane public, by renaming it to
>> bfd_section_size_insane.
>> 
>> I've updated the existing uses within bfd/, I don't believe this
>> function is used outside of bfd/ currently.
>> 
>> One place that I plan to make use of this function is in
>> gdb/gdb_bfd.c, in the function gdb_bfd_get_full_section_contents.
>> This change isn't included in this commit, but will come later if/when
>> this has been merged into bfd.
>
> Having seen your ping (and no other response), let me share my view:
> This function implements a certain policy, internal to the library.
> By exposing it, you would make external users dependent upon this
> specific policy. What if later we change our view on what's "insane"?

I would expect and want external users to get the updated definition.

The function name of "insane" is a little unfortunate.  I think if the
function had a better name then this change would seem far less
contentious.  Consider a name of:

  validate_section_size_against_other_bfd_infernal_properties_of_the_elf_to_ensure_that_the_requested_size_is_likely_valid()

> IOW external consumers want to implement their own, independent policy
> (if so desired).

Sure, consumers _could_ implement their own policy, but IMHO, this would
be far worse than exposing the *_insane() function.

What I (as a consumer) want is to check if the size that the BFD library
is reporting is valid or not.  To do that I need to check details of the
ELF that I, as a BFD users, shouldn't have to bother with. (I thought)
the point of BFD was to abstract details of the file format.

> Taking your intended usage example, things would be different if e.g.
> bfd_get_full_section_contents() itself used this check unconditionally.
> Then I could see a desire to have a way of checking up front whether
> allocating a buffer makes sense at all. And really I consider it
> questionable for bfd_get_full_section_contents(), when asked to
> allocate a buffer, to actually enforce such a library-internal policy.
> Like with exposing bfd_section_size_insane(), any change to the policy
> may affect external users in unexpected ways.

I don't understand this paragraph at all.  I'm sure I must be reading it
wrong, but it feels like you're saying we shouldn't use
bfd_section_size_insane(), which would mean we don't check for this one
particular error case, but I'm not sure why you'd feel that way.  Like I
said, I'm sure that's _not_ what you're suggesting, I just don't see
what it is you are trying to say.

You start this paragraph by saying "Taking your intended usage example,
..." but don't really offer an alternative solution.  I'd be interested
if you did have some thoughts.

Maybe a better solution is to change bfd_get_section_size() so that this
function doesn't always just return the recorded section size, but
instead returns 0 (or maybe -1 to indicate an error?) based on calling
bfd_section_size_insane()?  This feels far more risky as there's likely
many calls to bfd_section_size() in the wild that don't expect to get
back a size of 0.... but maybe that's a cleaner solution?

Or maybe we just need to rewrite this corner of GDB to avoid having GDB
allocate the buffers :/ Seems like an unfortunate conclusion...

Anyway, thanks for your thoughts,

Andrew