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

[Jan Beulich <jbeulich@suse.com>]

On 10.01.2024 12:03, Andrew Burgess wrote:
> 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.

And then break if we decide to lower the limit of "insane"?

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

Well, your wording (correctly) makes an important distinction: "valid" !=
"sane". If this was a validity check, no question would arise about it
being okay to expose.

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

The only alternative I can think about is for every component to enforce
its own view of "sane".

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

Indeed, this risk makes such a change undesirable. Plus when merely
dumping headers, for example, the true value will want returning (and
displaying) anyway. I was rather thinking the other way around, to
perhaps drop the "insane" checking, for being purely heuristic and
prone to break at some (hopefully distant) future point. A reasonably
well implemented allocation function ought to be able to fail without
first trying hard to free up memory, when enough cannot be made
available anyway.

Jan