[PATCH] elf-eh-frame, move buffer alloc out of if block

[PATCH] elf-eh-frame, move buffer alloc out of if block

[msnyder]

[-- Attachment #1: Type: text/plain, Size: 103 bytes --]

The else branch also relies on this pointer being non-null, so
just move the allocation above the if.


[-- Attachment #2: ecies.txt --]
[-- Type: text/plain, Size: 1477 bytes --]

2007-07-27  Michael Snyder  <msnyder@access-company.com>

	* elf-eh-frame.c (_bfd_elf_discard_section_eh_frame): Move alloc
	above if block, since both branches rely on it.

Index: elf-eh-frame.c
===================================================================
RCS file: /cvs/src/src/bfd/elf-eh-frame.c,v
retrieving revision 1.62
diff -p -r1.62 elf-eh-frame.c
*** elf-eh-frame.c	12 Jul 2007 07:16:41 -0000	1.62
--- elf-eh-frame.c	27 Jul 2007 21:18:16 -0000
*************** _bfd_elf_discard_section_eh_frame
*** 586,591 ****
--- 586,600 ----
        REQUIRE (skip_bytes (&buf, end, 4));
        hdr_id = bfd_get_32 (abfd, buf - 4);
  
+       if (ecie_count == ecie_alloced)
+ 	{
+ 	  ecies = bfd_realloc (ecies,
+ 			       (ecie_alloced + 20) * sizeof (*ecies));
+ 	  REQUIRE (ecies);
+ 	  memset (&ecies[ecie_alloced], 0, 20 * sizeof (*ecies));
+ 	  ecie_alloced += 20;
+ 	}
+ 
        if (hdr_id == 0)
  	{
  	  unsigned int initial_insn_length;
*************** _bfd_elf_discard_section_eh_frame
*** 593,607 ****
  	  /* CIE  */
  	  this_inf->cie = 1;
  
- 	  if (ecie_count == ecie_alloced)
- 	    {
- 	      ecies = bfd_realloc (ecies,
- 				   (ecie_alloced + 20) * sizeof (*ecies));
- 	      REQUIRE (ecies);
- 	      memset (&ecies[ecie_alloced], 0, 20 * sizeof (*ecies));
- 	      ecie_alloced += 20;
- 	    }
- 
  	  cie = &ecies[ecie_count].cie;
  	  ecies[ecie_count].offset = this_inf->offset;
  	  ecies[ecie_count++].entry = sec_info->count;
--- 602,607 ----

Re: [PATCH] elf-eh-frame, move buffer alloc out of if block

[Jakub Jelinek]

On Fri, Jul 27, 2007 at 02:21:19PM -0700, msnyder@sonic.net wrote:
> The else branch also relies on this pointer being non-null, so
> just move the allocation above the if.
> 

> 2007-07-27  Michael Snyder  <msnyder@access-company.com>
> 
> 	* elf-eh-frame.c (_bfd_elf_discard_section_eh_frame): Move alloc
> 	above if block, since both branches rely on it.

This is wrong.  Only if (hdr_id == 0) { ... } code ever adds new cies
to the array, else branch will just goto free_no_table; (failed REQUIRE)
if ecie_count == 0 (on an invalid .eh_frame section):

          /* Find the corresponding CIE.  */
          unsigned int cie_offset = this_inf->offset + 4 - hdr_id;
          for (ecie = ecies; ecie < ecies + ecie_count; ++ecie)
            if (cie_offset == ecie->offset)
              break;

          /* Ensure this FDE references one of the CIEs in this input
             section.  */
          REQUIRE (ecie != ecies + ecie_count);

So, if ecies is NULL (implies invalid .eh_frame section and also
ecie_count == 0), I don't see anything invalid on the
ecie = NULL assignment or NULL < NULL + 0 comparison (false), then it
will just do if (NULL == NULL + 0) goto free_no_table;

To my this looks like Coverity issue.

	Jakub

Re: [PATCH] elf-eh-frame, move buffer alloc out of if block

[msnyder]

> On Fri, Jul 27, 2007 at 02:21:19PM -0700, msnyder@sonic.net wrote:
>> The else branch also relies on this pointer being non-null, so
>> just move the allocation above the if.
>>
>
>> 2007-07-27  Michael Snyder  <msnyder@access-company.com>
>>
>> 	* elf-eh-frame.c (_bfd_elf_discard_section_eh_frame): Move alloc
>> 	above if block, since both branches rely on it.
>
> This is wrong.  Only if (hdr_id == 0) { ... } code ever adds new cies
> to the array, else branch will just goto free_no_table; (failed REQUIRE)
> if ecie_count == 0 (on an invalid .eh_frame section):
>
>           /* Find the corresponding CIE.  */
>           unsigned int cie_offset = this_inf->offset + 4 - hdr_id;
>           for (ecie = ecies; ecie < ecies + ecie_count; ++ecie)
>             if (cie_offset == ecie->offset)
>               break;
>
>           /* Ensure this FDE references one of the CIEs in this input
>              section.  */
>           REQUIRE (ecie != ecies + ecie_count);
>
> So, if ecies is NULL (implies invalid .eh_frame section and also
> ecie_count == 0), I don't see anything invalid on the
> ecie = NULL assignment or NULL < NULL + 0 comparison (false), then it
> will just do if (NULL == NULL + 0) goto free_no_table;

Well, that reasoning requires that you *know* that (ecies == NULL)
implies invalid .eh_frame section and ecie_count == 0.

OK then, how about this instead?

          /* Find the corresponding CIE.  */
          unsigned int cie_offset = this_inf->offset + 4 - hdr_id;
+         REQUIRE (ecies != NULL || ecie_count == 0);
          for (ecie = ecies; ecie < ecies + ecie_count; ++ecie)
            if (cie_offset == ecie->offset)
              break;

Re: [PATCH] elf-eh-frame, move buffer alloc out of if block

[Jakub Jelinek]

On Fri, Jul 27, 2007 at 04:52:10PM -0700, msnyder@sonic.net wrote:
> > So, if ecies is NULL (implies invalid .eh_frame section and also
> > ecie_count == 0), I don't see anything invalid on the
> > ecie = NULL assignment or NULL < NULL + 0 comparison (false), then it
> > will just do if (NULL == NULL + 0) goto free_no_table;
> 
> Well, that reasoning requires that you *know* that (ecies == NULL)
> implies invalid .eh_frame section and ecie_count == 0.

But sufficiently good analysis tool must be able to figure that out.
Initially ecies = NULL and ecie_count = 0 (var initialization at their
respective definitions).  The only place where ecie_count is increased
is after ecies = bfd_realloc () succeeded, at which point ecies != NULL.

So I'm not really sure we should work around Coverity inefficiencies.

You need to take the reported issues with a grain of salt, they show where
a problem might be.  You then analyze the thing and either assess there is
no problem and ideally report that to the provider of the tool, so that
they can improve it, or fix the problem.

	Jakub

Re: [PATCH] elf-eh-frame, move buffer alloc out of if block

[Michael Snyder]

> On Fri, Jul 27, 2007 at 04:52:10PM -0700, msnyder@sonic.net wrote:
> > > So, if ecies is NULL (implies invalid .eh_frame section and also
> > > ecie_count == 0), I don't see anything invalid on the
> > > ecie = NULL assignment or NULL < NULL + 0 comparison (false), then it
> > > will just do if (NULL == NULL + 0) goto free_no_table;
> >
> > Well, that reasoning requires that you *know* that (ecies == NULL)
> > implies invalid .eh_frame section and ecie_count == 0.
>
> But sufficiently good analysis tool must be able to figure that out.
> Initially ecies = NULL and ecie_count = 0 (var initialization at their
> respective definitions).  The only place where ecie_count is increased
> is after ecies = bfd_realloc () succeeded, at which point ecies != NULL.
>
> So I'm not really sure we should work around Coverity inefficiencies.
>
> You need to take the reported issues with a grain of salt, they show where
> a problem might be.  You then analyze the thing and either assess there is
> no problem and ideally report that to the provider of the tool, so that
> they can improve it, or fix the problem.

No worries -- I've reported back to them more than a dozen
false hits.