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 10.01.2024 17:20, Andrew Burgess wrote:
>> We're talking about the same _bfd_section_size_insane in bfd/section.c,
>> right?
>> 
>> I don't see any arbitrary limit in there.  If there is such a limit then
>> it's buried within all the very non-arbitrary sanity checks.
>> 
>> This function isn't answering: "is this section larger than X", it is
>> instead answering: "can this section possibly be read from this file".
>
> Hmm, first of all I have to apologize for having gone from memory, having
> looked at the function only when I first saw your patch. Yet then what
> you're talking about is merely the final check of the function, which I'd
> view as fine to externalize. The whole rest of the function still seems
> pretty (but not exclusively) heuristic to me.
>
> Anyway, Alan has indicated approval of making the function external, even
> as-is. If I was to make a suggestion beyond the possible renaming /
> inverting of sense, that would be to split the function into an "is valid"
> part and an "is sane" one, with the latter kept internal.

Hi Jan,

This patch dropped from my radar for a while, mostly because I didn't
really understand your last feedback -- I wanted to take some time then
revisit this patch with fresh eyes -- then I completely forgot about
this.

You talk about splitting the function into two parts, an "is valid" part
and an "is sane" part.  I've looked at _bfd_section_size_insane again,
and I still don't understand how you imagine the function being split.

Here's a high level description of the function as I see it:

  1. Get section size,

  2. If section is one that we "know" will always be sane (i.e. was
     created within a tool rather than read from a BFD) then this is a
     sane section,

  3. Get the file size,

  4. If the section is a compressed section then, do some check that I
     don't really understand (despite the comment) against the section
     size.  Later checks are done against the section's compressed size
     instead of the reported section size,

  5. If the section is defined as outside the bounds of the file, or the
     section size is larger than the file size then there must be
     something wrong with the section, and the section is declared
     "insane",

  6. Assume the section is sane.

There's a small unknown in step #4 that I don't understand, but
otherwise, this all seems pretty straight forward.  I don't really
understand how you imagine these parts being split.  And if they were
split, I don't understand why a user like GDB can't ask these questions
about a section (or rather, we can ask the same question, all the
functions/data that _bfd_section_size_insane uses are public, so GDB can
just copy & paste this code, but I'd rather not do that).

My motivation behind wanting to make this public is to allow GDB to
perform this sanity check before trying to allocate memory, which I
think will allow GDB to improve it's error handling when presented with
an invalid BFD (ELF) object.

Given Alan's feedback, I'm really just looking to see if you still have
objections/requests for this patch, or if you're happy for this to be
merged.

Thanks,
Andrew