llvm dev - Dec 2019 - [cfe-dev] clang and -D_FORTIFY_SOURCE=1

> Are you sure you've diagnosed the issue correctly?
__builtin___memcpy_chk
works correctly, as far as I know.

100% sure. Let's have a look at the output of

  #include <string.h>
  static char dest[10];
  char* square(int n) {
    memcpy(dest, "hello", n);
    return dest;
  }

compiled with -D_FORTIFY_SOURCE=1 -O1 : https://godbolt.org/z/UvABWp

Clang issues a call to memcpy, while gcc issues a call to __memcpy_chk.
The call to __memcpy_chk performs extra runtime checks memcpy doesn't,
and clang doesn't generate the extra checks inline either. This is a
separate
concern from the accuracy of __builtin_object_size, just a different
runtime behavior.

Clang could generate the call to __memcpy_chk if its declaration is
available, which is the case for the glibc.

On Tue, Dec 3, 2019 at 8:41 PM Eli Friedman <efriedma at quicinc.com>
wrote:
> Are you sure you've diagnosed the issue correctly?
__builtin___memcpy_chk
> works correctly, as far as I know.
>
> -Eli
>
> > -----Original Message-----
> > From: cfe-dev <cfe-dev-bounces at lists.llvm.org> On Behalf Of
Serge
> Guelton via
> > cfe-dev
> > Sent: Tuesday, December 3, 2019 2:07 AM
> > To: cfe-dev at lists.llvm.org
> > Cc: llvm-dev at lists.llvm.org
> > Subject: [EXT] [cfe-dev] clang and -D_FORTIFY_SOURCE=1
> >
> > Hi folks (CCing llvm-dev, but that's probably more of a cfe-dev
topic),
> >
> > As a follow-up to that old thread about -D_FORTIFY_SOURCE=n
> >
> >     http://lists.llvm.org/pipermail/cfe-dev/2015-November/045845.html
> >
> > And, more recently, to this fedora thread where clang/llvm -
> > D_FORTIFY_SOURCE
> > support is claimed to be only partial:
> >
> >     https://pagure.io/fesco/issue/2020
> >
> > I dig into the glibc headers in order to have a better understanding
of
> what's
> > going on, and wrote my notes here:
> >
> >
> https://sergesanspaille.fedorapeople.org/fortify_source_requirements.rst
> >
> > TL;DR: clang does provide a similar compile-time checking as gcc, but
no
> > runtime
> > checking. To assert that I wrote a small test suite:
> >
> >     https://github.com/serge-sans-paille/fortify-test-suite
> >
> > And indeed, clang doesn't pass it, mostly because it turns call to
> > __builtin__(.*)_chk into calls to __builtin__\1.
> >
> > We need to support the runtime behavior of the following builtins:
> >
> > - __builtin___memcpy_chk
> > - __builtin___memmove_chk
> > - __builtin___mempcpy_chk
> > - __builtin___memset_chk
> > - __builtin___snprintf_chk
> > - __builtin___sprintf_chk
> > - __builtin___stpcpy_chk
> > - __builtin___strcat_chk
> > - __builtin___strcpy_chk
> > - __builtin___strncat_chk
> > - __builtin___strncpy_chk
> > - __builtin___vsnprintf_chk
> > - __builtin___vsprintf_chk
> >
> > And I'd like to implement them at clang level, leveraging their
existing
> > implementation. Is that the right way to go / any comments / issue
with
> that
> > approach ?
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191204/dbdc9896/attachment.html>

On Wed, 4 Dec 2019, Serge Guelton via llvm-dev wrote:
> > Are you sure you've diagnosed the issue correctly?
__builtin___memcpy_chk
> works correctly, as far as I know.
> 100% sure. Let's have a look at the output of
> 
>   #include <string.h>
>   static char dest[10];
>   char* square(int n) {
>     memcpy(dest, "hello", n);
>     return dest;
>   }
> 
> compiled with -D_FORTIFY_SOURCE=1 -O1 : https://godbolt.org/z/UvABWp
> 
> Clang issues a call to memcpy, while gcc issues a call to __memcpy_chk.
> The call to __memcpy_chk performs extra runtime checks memcpy doesn't,
> and clang doesn't generate the extra checks inline either. This is a
> separate
> concern from the accuracy of __builtin_object_size, just a different
runtime
> behavior.
> 
> Clang could generate the call to __memcpy_chk if its declaration is
> available, which is the case for the glibc.
No, this doesn't seem to be the case.

I expanded your testcase by removing the string.h include and replacing it 
with the relevant bits of it:

https://godbolt.org/z/NKUQbW

The regular call to memcpy ends up without the extra safety checks and 
just generates a call to memcpy. But if I call __builtin___memcpy_chk 
directly, it does generate a call to __memcpy_chk.

So I would conclude that the builtin works just fine, but the issue is 
with the exact rules for when and how the inline function is preferred 
over the externally available version.

I added a "#define VISIBIILTY extern" to ease testing and changing it.
If
you change that into static, you'll see that the inline version of the 
function actually ends up used, and it does generate a call to 
__memcpy_chk even there.

And this does seem to show that __builtin_object_size does work fine from 
within an inlined function now, contrary to my earlier comments.

// Martin

> So I would conclude that the builtin works just fine, but the issue is
> with the exact rules for when and how the inline function is preferred
> over the externally available version.
Fascinating. I can elaborate a bit more based on
https://godbolt.org/z/EZGfqd : there's a special handling for memcpy (and
probably other similar functions) in clang that prevents him from
considering the inline version.I'll dig in clang source to find out more
about this behavior.

On Wed, Dec 4, 2019 at 9:11 AM Martin Storsjö <martin at martin.st> wrote:
> On Wed, 4 Dec 2019, Serge Guelton via llvm-dev wrote:
>
> > > Are you sure you've diagnosed the issue correctly?
> __builtin___memcpy_chk
> > works correctly, as far as I know.
> > 100% sure. Let's have a look at the output of
> >
> >   #include <string.h>
> >   static char dest[10];
> >   char* square(int n) {
> >     memcpy(dest, "hello", n);
> >     return dest;
> >   }
> >
> > compiled with -D_FORTIFY_SOURCE=1 -O1 : https://godbolt.org/z/UvABWp
> >
> > Clang issues a call to memcpy, while gcc issues a call to
__memcpy_chk.
> > The call to __memcpy_chk performs extra runtime checks memcpy
doesn't,
> > and clang doesn't generate the extra checks inline either. This is
a
> > separate
> > concern from the accuracy of __builtin_object_size, just a different
> runtime
> > behavior.
> >
> > Clang could generate the call to __memcpy_chk if its declaration is
> > available, which is the case for the glibc.
>
> No, this doesn't seem to be the case.
>
> I expanded your testcase by removing the string.h include and replacing it
> with the relevant bits of it:
>
> https://godbolt.org/z/NKUQbW
>
> The regular call to memcpy ends up without the extra safety checks and
> just generates a call to memcpy. But if I call __builtin___memcpy_chk
> directly, it does generate a call to __memcpy_chk.
>
> So I would conclude that the builtin works just fine, but the issue is
> with the exact rules for when and how the inline function is preferred
> over the externally available version.
>
> I added a "#define VISIBIILTY extern" to ease testing and
changing it. If
> you change that into static, you'll see that the inline version of the
> function actually ends up used, and it does generate a call to
> __memcpy_chk even there.
>
> And this does seem to show that __builtin_object_size does work fine from
> within an inlined function now, contrary to my earlier comments.
>
> // Martin
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191204/3d2925fa/attachment.html>