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

[Andrew Burgess]

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?

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

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?
Would you object to making the *_valid() name public?

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

And I'm 100% on board with the idea that BFD consumers might want to add
their own rules that determine good or bad sections.  But I don't
understand why you think the idea of having a baseline set of rules
within BFD makes no sense.  Surely this just forces every consumer to
copy the existing rules out of BFD and places a task on them to monitor
BFD and copy over any updates, which seems really wasteful...

... And sure, you can argue that a consumer might not want a particular
future change to *_insane(), but I feel such an argument is rather
contrived.  A BFD consumer might object to _any_ change in _any_ of
BFD's public APIs, that doesn't mean we don't make those changes in BFD,
nor does it mean that those consumers shouldn't use BFD.  I don't see
why _this_ particular API function is any different to any other API
function.

Thanks,
Andrew


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