[Bug libfortran/67535] New: write.c sanitizer detects null pointer passed to memcpy

[Bug libfortran/67535] New: write.c sanitizer detects null pointer passed to memcpy

[zeccav at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

            Bug ID: 67535
           Summary: write.c sanitizer detects null pointer passed to
                    memcpy
           Product: gcc
           Version: 5.2.0
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: libfortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zeccav at gmail dot com
  Target Milestone: ---

During "make check" a null pointer is sometimes passed to memcpy in
write.c:1877

memcpy (ext_name, base_name, base_name_len);

because base_name == NULL

but base_name_len == 0 so it should be harmless.
To make happy the sanitizer the statement should be

if(base_name_len) memcpy (ext_name, base_name, base_name_len);

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kargl at gcc dot gnu.org

--- Comment #1 from kargl at gcc dot gnu.org ---
(In reply to Vittorio Zecca from comment #0)
> During "make check" a null pointer is sometimes passed to memcpy in
> write.c:1877
> 
> memcpy (ext_name, base_name, base_name_len);
> 
> because base_name == NULL
> 
> but base_name_len == 0 so it should be harmless.
> To make happy the sanitizer the statement should be
> 
> if(base_name_len) memcpy (ext_name, base_name, base_name_len);

What happens to performance?  Simply making changes to
make sanitizer happy seems rather questionable.  It's clear
from context that if base_name == NULL, then base_name_len
== 0, and the memcpy should be a NOP.

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
It is still undefined behavior, so e.g. gcc will happily remove NULL pointer
checks after it sees a pointer used in memcpy, etc.

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[zeccav at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

--- Comment #3 from Vittorio Zecca <zeccav at gmail dot com> ---
(In reply to kargl from comment #1)

> What happens to performance?  Simply making changes to
> make sanitizer happy seems rather questionable.  It's clear
> from context that if base_name == NULL, then base_name_len
> == 0, and the memcpy should be a NOP.

How costly is that NOP compared to if(base_name_len)?

How costly is to let go an undefined behaviour?

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

--- Comment #4 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Thu, Sep 10, 2015 at 09:00:06AM +0000, zeccav at gmail dot com wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535
> 
> --- Comment #3 from Vittorio Zecca <zeccav at gmail dot com> ---
> (In reply to kargl from comment #1)
> 
> > What happens to performance?  Simply making changes to
> > make sanitizer happy seems rather questionable.  It's clear
> > from context that if base_name == NULL, then base_name_len
> > == 0, and the memcpy should be a NOP.
> 
> How costly is that NOP compared to if(base_name_len)?

I don't know.  Of course, I'm not the one purposing a
change.

> 
> How costly is to let go an undefined behaviour?
> 

It's undefined behavior to pass a NULL pointer into a function?

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Steve Kargl from comment #4)
> It's undefined behavior to pass a NULL pointer into a function?

To a function that does not allow it?  Yes.

Citing the C standard:
7.21.2.1/2:
"The memcpy function copies n characters from the object pointed to by s2 into
the object pointed to by s1."
(NULL pointer does not point to any object).
7.1.4/1:
"If a function argument is
described as being an array, the pointer actually passed to the function shall
have a value
such that all address computations and accesses to objects (that would be valid
if the
pointer did point to the first element of such an array) are in fact valid."
7.21.1:
"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."

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[zeccav at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

--- Comment #6 from Vittorio Zecca <zeccav at gmail dot com> ---
The cost of adding "if(base_name_len)" is two x86-64 machine instructions

        cmpl    $0, -20(%rbp)
        je      .L2

Six instructions follow then

        call memcpy

which is not exactly a NOP even with base_name_len==0

In my opinion two machine instructions to avoid a useless and undefined memcpy
are a bargain.

It is not undefined behaviour to pass a NULL pointer to an arbitrary function,
but it is undefined to pass it to memcpy.

And it seems to me that after
memcpy(dest,src,len)

the statement "if(src)" is always TRUE because the optimizer assumes src!=NULL

The following is from https://gcc.gnu.org/gcc-4.9/porting_to.html

Null pointer checks may be optimized away more aggressively

GCC might now optimize away the null pointer check in code like:


  int copy (int* dest, int* src, size_t nbytes) {
    memmove (dest, src, nbytes);
    if (src != NULL)
      return *src;
    return 0;
  }

The pointers passed to memmove (and similar functions in <string.h>) must be
non-null even when nbytes==0, so GCC can use that information to remove the
check after the memmove call. Calling copy(p, NULL, 0) can therefore deference
a null pointer and crash.

The example above needs to be fixed to avoid the invalid memmove call, for
example:


    if (nbytes != 0)
      memmove (dest, src, nbytes);

This optimization can also affect implicit null pointer checks such as the one
done by the C++ runtime for the delete[] operator.

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

--- Comment #7 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Author: fxcoudert
Date: Sat Sep 12 12:05:44 2015
New Revision: 227705

URL: https://gcc.gnu.org/viewcvs?rev=227705&root=gcc&view=rev
Log:
        PR libfortran/67527
        PR libfortran/67535
        PR libfortran/67536
        * io/io.h: Use unsigned values for 31-bit left shifts.
        * io/unix.c (buf_read): Do not call memcpy() with NULL pointer arg.
        * io/write.c (nml_write_obj): Likewise.

Modified:
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/io/io.h
    trunk/libgfortran/io/unix.c
    trunk/libgfortran/io/write.c

[Bug libfortran/67535] write.c sanitizer detects null pointer passed to memcpy

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67535

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
                 CC|                            |fxcoudert at gcc dot gnu.org
         Resolution|---                         |FIXED
           Assignee|unassigned at gcc dot gnu.org      |fxcoudert at gcc dot gnu.org
   Target Milestone|---                         |6.0

--- Comment #8 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Fixed. Thanks for the report.