[RFA] Avoid ubsan complaint in BFD

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFA] Avoid ubsan complaint in BFD

Tom Tromey-2
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

Re: [RFA] Avoid ubsan complaint in BFD

John Darrington-4
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  <[hidden email]>
     
      * 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.


signature.asc (188 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFA] Avoid ubsan complaint in BFD

Tom Tromey-2
>>>>> "John" == John Darrington <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [RFA] Avoid ubsan complaint in BFD

Jeff Law
On 07/20/2018 11:48 AM, Tom Tromey wrote:

>>>>>> "John" == John Darrington <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [RFA] Avoid ubsan complaint in BFD

Nick Clifton
In reply to this post by Tom Tromey-2
Hi Tom,

> bfd/ChangeLog
> 2018-07-20  Tom Tromey  <[hidden email]>
>
> * 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