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

[Jan Beulich <jbeulich@suse.com>]

On 10.01.2024 14:48, Andrew Burgess wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> 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"?
> 
> You've said this again later in your reply, and I think this is the bit
> that I'm having trouble with.
> 
> Why do you think things would break if the definition of "insane"
> changed?
> 
> Of course, I ask this under the assumption that there's no broken code
> in the consumer, clearly:
> 
>   if (bfd_section_size_insane (...))
>     // Broken code here...
>   else
>     // Working code here...
> 
> Is going to break, and if *_insane() changes, then we're going to break
> in more or less cases, but that seems slightly contrived.
> 
> More likely we're going to write:
> 
>   if (bfd_section_size_insane (...))
>     // Handle cases where the section is invalid.
>   else
>     // Load the section and process it.
> 
> If the definition of *_insane() changes then I 100% want to enter the
> error case more often.
> 
> Of course, you might say, but what if the code was working fine before?
> Then changing *_insane() is sending more cases down the error path...
> 
> ... but I would suggest that this would be an indication of a bug being
> introduced into *_insane(), I mean, if such a thing happened then we'd
> already run into problems.
> 
> So, in summary, the bit I'm not understanding here is why you feel
> future changes to *_insane() would result in breakage in the consumer?

Let's assume "insane" is anything above 4G right now (I don't recall what
the real value is), and people are working with 3G objects, using any
arbitrary consumer of libbfd. Then we choose lo lower the limit to 2G.

While surely the limit is higher than the given example, the pattern
remains: People working fine with the present limit might find themselves
hosed if that limit was lowered. Hence why I dislike such heuristics, in
turn meaning I would like to not see them put into wider use.

>>> 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.
> 
> I didn't intend to make any such distinction.  I was trying to say that
> this function _is_ a validity check, it's just unfortunately named as a
> sanity check.

This is then where we disagree. There's no limit the ELF spec imposes
on sh_size, just to take the most common (for GNU binutils) example.
Hence there simply are no invalid size values (as far as ELF is
concerned), and libbfd shouldn't make up any.

> If I proposed renaming the function to 'is_bfd_section_size_valid()'
> but left the implementation the same would you see a problem with this?

Yes, because the size check failing _does not_ mean the size is invalid.
Memory sizes grow all the time, storage sizes grow all the time, and
object file as well as section sizes also (almost) only ever grow.

> Would you object to making the *_valid() name public?

If the function truly performed validity checks, of course I wouldn't
object.

Jan