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

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

[Andrew Burgess]

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.

There should be no change in behaviour after this commit.
---
 bfd/bfd-in2.h  | 2 ++
 bfd/compress.c | 4 ++--
 bfd/dwarf2.c   | 4 ++--
 bfd/libbfd.h   | 2 --
 bfd/section.c  | 8 ++++----
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 040d5560cdf..c3ade639ad7 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -1088,6 +1088,8 @@ const char *bfd_generic_group_name (bfd *, const asection *sec);
 
 bool bfd_generic_discard_group (bfd *abfd, asection *group);
 
+bool bfd_section_size_insane (bfd *abfd, asection *sec);
+
 /* Extracted from syms.c.  */
 typedef struct bfd_symbol
 {
diff --git a/bfd/compress.c b/bfd/compress.c
index 7ec7b0edf38..17f996f1613 100644
--- a/bfd/compress.c
+++ b/bfd/compress.c
@@ -735,7 +735,7 @@ bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)
 
   if (p == NULL
       && compress_status != COMPRESS_SECTION_DONE
-      && _bfd_section_size_insane (abfd, sec))
+      && bfd_section_size_insane (abfd, sec))
     {
       /* PR 24708: Avoid attempts to allocate a ridiculous amount
 	 of memory.  */
@@ -1066,7 +1066,7 @@ bfd_init_section_compress_status (bfd *abfd, sec_ptr sec)
       || sec->rawsize != 0
       || sec->contents != NULL
       || sec->compress_status != COMPRESS_SECTION_NONE
-      || _bfd_section_size_insane (abfd, sec))
+      || bfd_section_size_insane (abfd, sec))
     {
       bfd_set_error (bfd_error_invalid_operation);
       return false;
diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index 22e6b467598..771929a8c28 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -716,7 +716,7 @@ read_section (bfd *abfd,
 	  return false;
 	}
 
-      if (_bfd_section_size_insane (abfd, msec))
+      if (bfd_section_size_insane (abfd, msec))
 	{
 	  /* PR 26946 */
 	  _bfd_error_handler (_("DWARF error: section %s is too big"),
@@ -5498,7 +5498,7 @@ _bfd_dwarf2_slurp_debug_info (bfd *abfd, bfd *debug_bfd,
 	   msec;
 	   msec = find_debug_info (debug_bfd, debug_sections, msec))
 	{
-	  if (_bfd_section_size_insane (debug_bfd, msec))
+	  if (bfd_section_size_insane (debug_bfd, msec))
 	    goto restore_vma;
 	  /* Catch PR25070 testcase overflowing size calculation here.  */
 	  if (total_size + msec->size < total_size)
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index cc432677a81..c7ae249dfea 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -3641,8 +3641,6 @@ bool _bfd_unrecognized_reloc
     unsigned int r_type) ATTRIBUTE_HIDDEN;
 
 /* Extracted from section.c.  */
-bool _bfd_section_size_insane (bfd *abfd, asection *sec) ATTRIBUTE_HIDDEN;
-
 /* Extracted from stabs.c.  */
 bool _bfd_link_section_stabs
    (bfd *, struct stab_info *, asection *, asection *, void **,
diff --git a/bfd/section.c b/bfd/section.c
index e9af59dfd15..c9e5d3e0b65 100644
--- a/bfd/section.c
+++ b/bfd/section.c
@@ -1715,11 +1715,11 @@ _bfd_nowrite_set_section_contents (bfd *abfd,
 }
 
 /*
-INTERNAL_FUNCTION
-	_bfd_section_size_insane
+FUNCTION
+	bfd_section_size_insane
 
 SYNOPSIS
-	bool _bfd_section_size_insane (bfd *abfd, asection *sec);
+	bool bfd_section_size_insane (bfd *abfd, asection *sec);
 
 DESCRIPTION
 	Returns true if the given section has a size that indicates
@@ -1729,7 +1729,7 @@ DESCRIPTION
 */
 
 bool
-_bfd_section_size_insane (bfd *abfd, asection *sec)
+bfd_section_size_insane (bfd *abfd, asection *sec)
 {
   bfd_size_type size = bfd_get_section_limit_octets (abfd, sec);
   if (size == 0)

base-commit: 5a22e042e41db962cd6a79cd59cab46cbbe58a98
-- 
2.25.4

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

[Andrew Burgess]

Ping!

Thanks,
Andrew


Andrew Burgess <aburgess@redhat.com> writes:

> 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.
>
> There should be no change in behaviour after this commit.
> ---
>  bfd/bfd-in2.h  | 2 ++
>  bfd/compress.c | 4 ++--
>  bfd/dwarf2.c   | 4 ++--
>  bfd/libbfd.h   | 2 --
>  bfd/section.c  | 8 ++++----
>  5 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index 040d5560cdf..c3ade639ad7 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -1088,6 +1088,8 @@ const char *bfd_generic_group_name (bfd *, const asection *sec);
>  
>  bool bfd_generic_discard_group (bfd *abfd, asection *group);
>  
> +bool bfd_section_size_insane (bfd *abfd, asection *sec);
> +
>  /* Extracted from syms.c.  */
>  typedef struct bfd_symbol
>  {
> diff --git a/bfd/compress.c b/bfd/compress.c
> index 7ec7b0edf38..17f996f1613 100644
> --- a/bfd/compress.c
> +++ b/bfd/compress.c
> @@ -735,7 +735,7 @@ bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)
>  
>    if (p == NULL
>        && compress_status != COMPRESS_SECTION_DONE
> -      && _bfd_section_size_insane (abfd, sec))
> +      && bfd_section_size_insane (abfd, sec))
>      {
>        /* PR 24708: Avoid attempts to allocate a ridiculous amount
>  	 of memory.  */
> @@ -1066,7 +1066,7 @@ bfd_init_section_compress_status (bfd *abfd, sec_ptr sec)
>        || sec->rawsize != 0
>        || sec->contents != NULL
>        || sec->compress_status != COMPRESS_SECTION_NONE
> -      || _bfd_section_size_insane (abfd, sec))
> +      || bfd_section_size_insane (abfd, sec))
>      {
>        bfd_set_error (bfd_error_invalid_operation);
>        return false;
> diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
> index 22e6b467598..771929a8c28 100644
> --- a/bfd/dwarf2.c
> +++ b/bfd/dwarf2.c
> @@ -716,7 +716,7 @@ read_section (bfd *abfd,
>  	  return false;
>  	}
>  
> -      if (_bfd_section_size_insane (abfd, msec))
> +      if (bfd_section_size_insane (abfd, msec))
>  	{
>  	  /* PR 26946 */
>  	  _bfd_error_handler (_("DWARF error: section %s is too big"),
> @@ -5498,7 +5498,7 @@ _bfd_dwarf2_slurp_debug_info (bfd *abfd, bfd *debug_bfd,
>  	   msec;
>  	   msec = find_debug_info (debug_bfd, debug_sections, msec))
>  	{
> -	  if (_bfd_section_size_insane (debug_bfd, msec))
> +	  if (bfd_section_size_insane (debug_bfd, msec))
>  	    goto restore_vma;
>  	  /* Catch PR25070 testcase overflowing size calculation here.  */
>  	  if (total_size + msec->size < total_size)
> diff --git a/bfd/libbfd.h b/bfd/libbfd.h
> index cc432677a81..c7ae249dfea 100644
> --- a/bfd/libbfd.h
> +++ b/bfd/libbfd.h
> @@ -3641,8 +3641,6 @@ bool _bfd_unrecognized_reloc
>      unsigned int r_type) ATTRIBUTE_HIDDEN;
>  
>  /* Extracted from section.c.  */
> -bool _bfd_section_size_insane (bfd *abfd, asection *sec) ATTRIBUTE_HIDDEN;
> -
>  /* Extracted from stabs.c.  */
>  bool _bfd_link_section_stabs
>     (bfd *, struct stab_info *, asection *, asection *, void **,
> diff --git a/bfd/section.c b/bfd/section.c
> index e9af59dfd15..c9e5d3e0b65 100644
> --- a/bfd/section.c
> +++ b/bfd/section.c
> @@ -1715,11 +1715,11 @@ _bfd_nowrite_set_section_contents (bfd *abfd,
>  }
>  
>  /*
> -INTERNAL_FUNCTION
> -	_bfd_section_size_insane
> +FUNCTION
> +	bfd_section_size_insane
>  
>  SYNOPSIS
> -	bool _bfd_section_size_insane (bfd *abfd, asection *sec);
> +	bool bfd_section_size_insane (bfd *abfd, asection *sec);
>  
>  DESCRIPTION
>  	Returns true if the given section has a size that indicates
> @@ -1729,7 +1729,7 @@ DESCRIPTION
>  */
>  
>  bool
> -_bfd_section_size_insane (bfd *abfd, asection *sec)
> +bfd_section_size_insane (bfd *abfd, asection *sec)
>  {
>    bfd_size_type size = bfd_get_section_limit_octets (abfd, sec);
>    if (size == 0)
>
> base-commit: 5a22e042e41db962cd6a79cd59cab46cbbe58a98
> -- 
> 2.25.4

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

[Jan Beulich]

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"?
IOW external consumers want to implement their own, independent policy
(if so desired).

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.

Jan

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

[Andrew Burgess]

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

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

[Jan Beulich]

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

Re: [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

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

[Jan Beulich]

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

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

[Andrew Burgess]

Jan Beulich <jbeulich@suse.com> writes:

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

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

It doesn't matter how much memory your future computer has, if the ELF
is 1M in size, and the section claims to be 2M in size, you aren't going
to be reading that section out of that ELF.

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

Are you sure?  The "spec" (I use quotes because the only ELF spec I've
ever read is pretty light weight) might not say it, but an sh_size for a
section that is bigger than the ELF itself sounds pretty invalid too me.

[ Yes: I'm ignoring !SHT_NOBITS, and compressed sections here for
  simplicity. ]

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

My claim is that the function, as written today, is a validity check.

The only bit I think which could be described as arbitrary relates to
the compressed section handling.  I don't fully understand the comment
there, but the comment is pretty clear the 10x is an arbitrary heuristic
rather than a calculated value.  Obviously this could be improved if it
ever became a problem (calculate actual compressed/uncompressed sizes).

You worries don't seem to align with what I think the function actually
does.

Thanks,
Andrew

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

[Alan Modra]

On Wed, Jan 10, 2024 at 04:20:19PM +0000, Andrew Burgess wrote:
> My claim is that the function, as written today, is a validity check.

Yes, it is a sanity check, hence the name.  I'm OK with making this
function available outside of BFD, and if "insane" in the name is
uncomfortable then changing it to "bfd_section_size_sane" and
inverting the logic is fine by me too.

-- 
Alan Modra
Australia Development Lab, IBM

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

[Jan Beulich]

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.

Jan

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

[Andrew Burgess]

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

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

[Jan Beulich]

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

Given Alan's feedback, honestly I expected you long put that change in.
Feel free to. Being "happy" about it is perhaps a little much to say,
but I'm okay with you doing so.

Jan

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

[Andrew Burgess]

Jan Beulich <jbeulich@suse.com> writes:

> On 06.03.2024 12:17, Andrew Burgess wrote:
>> 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.
>
> Given Alan's feedback, honestly I expected you long put that change in.
> Feel free to. Being "happy" about it is perhaps a little much to say,
> but I'm okay with you doing so.

I've gone ahead and merged this patch.

Thanks,
Andrew

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

[Tom Tromey]

>>>>> "Jan" == Jan Beulich <jbeulich@suse.com> writes:

Jan> Having seen your ping (and no other response), let me share my view:
Jan> This function implements a certain policy, internal to the library.
Jan> By exposing it, you would make external users dependent upon this
Jan> specific policy. What if later we change our view on what's "insane"?
Jan> IOW external consumers want to implement their own, independent policy
Jan> (if so desired).

Merely exposing the API does not require any BFD user to actually call
it.

Furthermore it seems fine if it changes.  And, if it isn't fine in some
case, then that situation can be addressed when this actually happens.
TBH This seems unlikely to me and anyway doesn't seem like a very strong
reason to prevent the function from being made public.

Tom