[PATCH] aout relocs

[PATCH] aout relocs

[msnyder]

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

Check me on this one, I'm a little uncertain.

As near as I can tell, if reloc_size is zero, the routine does
nothing useful.  Maybe it will never be zero, but if it is, a few
iffy things will happen:

 * we'll call malloc with a size of zero, which is ill defined,
and later free the result

 * we'll call bfd_bread with a size of zero, and

 * a potentially null pointer may slip thru the cracks.


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

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

	* aoutx.h (NAME): Return TRUE if reloc_size is zero.

Index: aoutx.h
===================================================================
RCS file: /cvs/src/src/bfd/aoutx.h,v
retrieving revision 1.66
diff -p -r1.66 aoutx.h
*** aoutx.h	3 Jul 2007 14:26:39 -0000	1.66
--- aoutx.h	25 Jul 2007 18:59:09 -0000
*************** NAME (aout, slurp_reloc_table) (bfd *abf
*** 2280,2285 ****
--- 2280,2288 ----
        return FALSE;
      }
  
+   if (reloc_size == 0)
+     return TRUE;		/* Nothing to be done.  */
+ 
    if (bfd_seek (abfd, asect->rel_filepos, SEEK_SET) != 0)
      return FALSE;
  
*************** NAME (aout, slurp_reloc_table) (bfd *abf
*** 2289,2299 ****
  
    amt = count * sizeof (arelent);
    reloc_cache = bfd_zmalloc (amt);
!   if (reloc_cache == NULL && count != 0)
      return FALSE;
  
    relocs = bfd_malloc (reloc_size);
!   if (relocs == NULL && reloc_size != 0)
      {
        free (reloc_cache);
        return FALSE;
--- 2292,2302 ----
  
    amt = count * sizeof (arelent);
    reloc_cache = bfd_zmalloc (amt);
!   if (reloc_cache == NULL)
      return FALSE;
  
    relocs = bfd_malloc (reloc_size);
!   if (relocs == NULL)
      {
        free (reloc_cache);
        return FALSE;

Re: [PATCH] aout relocs

[Ian Lance Taylor]

msnyder@sonic.net writes:

> As near as I can tell, if reloc_size is zero, the routine does
> nothing useful.  Maybe it will never be zero, but if it is, a few
> iffy things will happen:
> 
>  * we'll call malloc with a size of zero, which is ill defined,
> and later free the result

No, we'll call bfd_malloc with a size of zero.  That is not
ill-defined.  It will either return NULL, or not, as (confusingly)
specified in the C standard.  Passing a NULL pointer to free will
always work.

>  * we'll call bfd_bread with a size of zero, and

Which is fine.

>  * a potentially null pointer may slip thru the cracks.

I'm not sure which pointer you mean here.

> 2007-07-25  Michael Snyder  <msnyder@access-company.com>
> 
> 	* aoutx.h (NAME): Return TRUE if reloc_size is zero.

I have no particular objection to this patch, but the ChangeLog entry
is wrong.  Emacs ^x 4 a will misfire in these functions; you have to
fill in the name manually.

I have to ask, though: why a.out?

Ian

Re: [PATCH] aout relocs

[msnyder]

> msnyder@sonic.net writes:
>
>> As near as I can tell, if reloc_size is zero, the routine does
>> nothing useful.  Maybe it will never be zero, but if it is, a few
>> iffy things will happen:
>>
>>  * we'll call malloc with a size of zero, which is ill defined,
>> and later free the result
>
> No, we'll call bfd_malloc with a size of zero.  That is not
> ill-defined.

With hat in hand, are you sure?  bfd_malloc does not check for
size == 0 before it calls malloc, and malloc(0) is "implementation
defined" (whatever that may mean).

> It will either return NULL, or not, as (confusingly)
> specified in the C standard.  Passing a NULL pointer to free will
> always work.
>
>>  * we'll call bfd_bread with a size of zero, and
>
> Which is fine.

Again, are you sure?  bfd_bread doesn't seem to check size either,
and it passes it to memcpy.  Is memcpy(x,y,0) well defined?

>>  * a potentially null pointer may slip thru the cracks.
>
> I'm not sure which pointer you mean here.

OK: we have
  relocs = bfd_malloc (reloc_size);  // which might be zero
  if (relocs == NULL && reloc_size != 0)
    bail;

So now it is possible that relocs == NULL and reloc_size == 0.
And then,

  if (bfd_bread (relocs, reloc_size, abfd)...

And bfd_bread does this:

  memcpy (ptr, bim->buffer + abfd->where, size);

where both ptr and size might be zero.

Note, sorry about the changelog, I'll take care of that.

> Why aout?

I know, I know...   I'm looking at a Coverity scan.

Re: [PATCH] aout relocs

[Andreas Schwab]

msnyder@sonic.net writes:

> With hat in hand, are you sure?  bfd_malloc does not check for
> size == 0 before it calls malloc, and malloc(0) is "implementation
> defined" (whatever that may mean).

There are only two ways malloc(0) may behave: either return NULL or a
unique pointer.

> Is memcpy(x,y,0) well defined?

Sure.

> And bfd_bread does this:
>
>   memcpy (ptr, bim->buffer + abfd->where, size);
>
> where both ptr and size might be zero.

That's the only place where something needs to be done (the pointer must
still be valid even if the size is zero).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] aout relocs

[msnyder]

>> And bfd_bread does this:
>>
>>   memcpy (ptr, bim->buffer + abfd->where, size);
>>
>> where both ptr and size might be zero.
>
> That's the only place where something needs to be done (the pointer must
> still be valid even if the size is zero).

Alright -- that's enough to justify the patch, isn't it?

Re: [PATCH] aout relocs

[Nick Clifton]

Hi Michael,

>> That's the only place where something needs to be done (the pointer must
>> still be valid even if the size is zero).
> 
> Alright -- that's enough to justify the patch, isn't it?

Certainly - please apply it.  Although you could simplify the second part of 
the patch by just checking to see if count is zero and returning, in the same 
way as you check reloc_size.  That way you do not need to check the return 
values of both allocs.

Cheers
   Nick

Re: [PATCH] aout relocs

[msnyder]

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

> Hi Michael,
>
>>> That's the only place where something needs to be done (the pointer
>>> must
>>> still be valid even if the size is zero).
>>
>> Alright -- that's enough to justify the patch, isn't it?
>
> Certainly - please apply it.  Although you could simplify the second part
> of
> the patch by just checking to see if count is zero and returning, in the
> same
> way as you check reloc_size.

Done.

> That way you do not need to check the return values of both allocs.

Well, yes I do -- bfd_malloc can still fail.
Checked in as attached.


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

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

	* aoutx.h (slurp_reloc_table): Return TRUE if reloc_size == zero
	or count == zero.

Index: aoutx.h
===================================================================
RCS file: /cvs/src/src/bfd/aoutx.h,v
retrieving revision 1.66
diff -p -r1.66 aoutx.h
*** aoutx.h	3 Jul 2007 14:26:39 -0000	1.66
--- aoutx.h	26 Jul 2007 18:27:21 -0000
*************** NAME (aout, slurp_reloc_table) (bfd *abf
*** 2280,2299 ****
        return FALSE;
      }
  
    if (bfd_seek (abfd, asect->rel_filepos, SEEK_SET) != 0)
      return FALSE;
  
    each_size = obj_reloc_entry_size (abfd);
  
    count = reloc_size / each_size;
  
    amt = count * sizeof (arelent);
    reloc_cache = bfd_zmalloc (amt);
!   if (reloc_cache == NULL && count != 0)
      return FALSE;
  
    relocs = bfd_malloc (reloc_size);
!   if (relocs == NULL && reloc_size != 0)
      {
        free (reloc_cache);
        return FALSE;
--- 2280,2304 ----
        return FALSE;
      }
  
+   if (reloc_size == 0)
+     return TRUE;		/* Nothing to be done.  */
+ 
    if (bfd_seek (abfd, asect->rel_filepos, SEEK_SET) != 0)
      return FALSE;
  
    each_size = obj_reloc_entry_size (abfd);
  
    count = reloc_size / each_size;
+   if (count == 0)
+     return TRUE;		/* Nothing to be done.  */
  
    amt = count * sizeof (arelent);
    reloc_cache = bfd_zmalloc (amt);
!   if (reloc_cache == NULL)
      return FALSE;
  
    relocs = bfd_malloc (reloc_size);
!   if (relocs == NULL)
      {
        free (reloc_cache);
        return FALSE;