[RFA] Avoid ubsan complaint in BFD

Message ID 20180720155241.20974-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Avoid ubsan complaint in BFD
Related show

Commit Message

Tom Tromey July 20, 2018, 3:52 p.m.
I built gdb with ubsan and ran the test suite.

One complaint was due to bfd_get_elf_phdrs passing NULL to memcpy.
This patch avoids the complaint.

bfd/ChangeLog
2018-07-20  Tom Tromey  <tom@tromey.com>

	* elf.c (bfd_get_elf_phdrs): Don't call memcpy with size 0.
---
 bfd/ChangeLog | 4 ++++
 bfd/elf.c     | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.13.6

Comments

John Darrington July 20, 2018, 5:12 p.m. | #1
My personal opinion is that if tools don't understand the standard(s)
properly, then it's bad form to pander to them just for the sake of
shutting them up.

I remember in years gone by people used to religiously write
if (x != NULL)
  free (x);

As well as needlessly increasting the line count it meant that coverage
analysis would complain about untrialled branches.  It got to be silly.

Just my $0.02

On Fri, Jul 20, 2018 at 09:52:41AM -0600, Tom Tromey wrote:
     I built gdb with ubsan and ran the test suite.
     
     One complaint was due to bfd_get_elf_phdrs passing NULL to memcpy.
     This patch avoids the complaint.
     
     bfd/ChangeLog
     2018-07-20  Tom Tromey  <tom@tromey.com>
     
     	* elf.c (bfd_get_elf_phdrs): Don't call memcpy with size 0.
     ---
      bfd/ChangeLog | 4 ++++
      bfd/elf.c     | 5 +++--
      2 files changed, 7 insertions(+), 2 deletions(-)
     
     diff --git a/bfd/elf.c b/bfd/elf.c
     index 874629dc859..f72182788f9 100644
     --- a/bfd/elf.c
     +++ b/bfd/elf.c
     @@ -11629,8 +11629,9 @@ bfd_get_elf_phdrs (bfd *abfd, void *phdrs)
          }
      
        num_phdrs = elf_elfheader (abfd)->e_phnum;
     -  memcpy (phdrs, elf_tdata (abfd)->phdr,
     -	  num_phdrs * sizeof (Elf_Internal_Phdr));
     +  if (num_phdrs != 0)
     +    memcpy (phdrs, elf_tdata (abfd)->phdr,
     +	    num_phdrs * sizeof (Elf_Internal_Phdr));
      
        return num_phdrs;
      }
     -- 
     2.13.6

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.
Tom Tromey July 20, 2018, 5:48 p.m. | #2
>>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:


John> My personal opinion is that if tools don't understand the standard(s)
John> properly, then it's bad form to pander to them just for the sake of
John> shutting them up.

I'm not really that great at reading the C standard, but I think it says
that passing NULL to memcpy is undefined behavior.

I did some research and came up with this.
Reading from n1570.pdf, section 7.24.1 "String function conventions":

    Where an argument declared as size_t n specifies the length of the
    array for a function, n can have the value zero on a call to that
    function.  Unless explicitly stated otherwise in the description of
    a particular function in this subclause, pointer arguments on such a
    call shall still have valid values, as described in 7.1.4.  On such
    a call, a function that locates a character finds no occurrence, a
    function that compares two character sequences returns zero, and a
    function that copies characters copies zero characters.

This applies to memcpy, which is defined in section 7.24.2.1.
And, there is no clause there saying that NULL is allowed.

glibc seems to agree with this, as it marks these arguments __nonnull.

If you think this is in error, I'd appreciate learning why.

thanks,
Tom
Jeff Law July 20, 2018, 5:57 p.m. | #3
On 07/20/2018 11:48 AM, Tom Tromey wrote:
>>>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:

> 

> John> My personal opinion is that if tools don't understand the standard(s)

> John> properly, then it's bad form to pander to them just for the sake of

> John> shutting them up.

> 

> I'm not really that great at reading the C standard, but I think it says

> that passing NULL to memcpy is undefined behavior.

> 

> I did some research and came up with this.

> Reading from n1570.pdf, section 7.24.1 "String function conventions":

> 

>     Where an argument declared as size_t n specifies the length of the

>     array for a function, n can have the value zero on a call to that

>     function.  Unless explicitly stated otherwise in the description of

>     a particular function in this subclause, pointer arguments on such a

>     call shall still have valid values, as described in 7.1.4.  On such

>     a call, a function that locates a character finds no occurrence, a

>     function that compares two character sequences returns zero, and a

>     function that copies characters copies zero characters.

> 

> This applies to memcpy, which is defined in section 7.24.2.1.

> And, there is no clause there saying that NULL is allowed.

> 

> glibc seems to agree with this, as it marks these arguments __nonnull.

> 

> If you think this is in error, I'd appreciate learning why.

Passing NULL to the mem* functions is invalid, even when the size of the
copy request is zero.  The clauses above are the same ones I used when
working through this issue a couple years ago.

It has been argued that ISO should make an exception, particularly for
memcpy, memmove and memset with size 0.  I've signaled to various folks
including Red Hat's rep on the ISO committee that I would support that
kind of exception -- the amount of broken code out there is significant.
 The consequences of such broken code are subtle, but important from a
security standpoint given how compilers such as GCC may utilize the
nonnull attribute.

Jeff
Nick Clifton July 23, 2018, 10:02 a.m. | #4
Hi Tom,

> bfd/ChangeLog

> 2018-07-20  Tom Tromey  <tom@tromey.com>

> 

> 	* elf.c (bfd_get_elf_phdrs): Don't call memcpy with size 0.


Approved, please apply.

I am sidestepping the debate on whether memcpy should handle a size of 0 or not ...

Cheers
  Nick

Patch

diff --git a/bfd/elf.c b/bfd/elf.c
index 874629dc859..f72182788f9 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -11629,8 +11629,9 @@  bfd_get_elf_phdrs (bfd *abfd, void *phdrs)
     }
 
   num_phdrs = elf_elfheader (abfd)->e_phnum;
-  memcpy (phdrs, elf_tdata (abfd)->phdr,
-	  num_phdrs * sizeof (Elf_Internal_Phdr));
+  if (num_phdrs != 0)
+    memcpy (phdrs, elf_tdata (abfd)->phdr,
+	    num_phdrs * sizeof (Elf_Internal_Phdr));
 
   return num_phdrs;
 }