[PATCH] strace: Fix crash caused over-optimization

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] strace: Fix crash caused over-optimization

Daniel Santos
Recent versions of gcc are optimizing away the TLS buffer allocated in
main, so we need to tell gcc that it's really used.
---
 winsup/utils/strace.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
index beab67b90..1e581b4a4 100644
--- a/winsup/utils/strace.cc
+++ b/winsup/utils/strace.cc
@@ -1192,6 +1192,8 @@ main (int argc, char **argv)
   char buf[CYGTLS_PADSIZE];
 
   memset (buf, 0, sizeof (buf));
+  /* Prevent buf from being optimized away.  */
+  __asm__ __volatile__("" :: "m" (buf));
   exit (main2 (argc, argv));
 }
 
--
2.11.0

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] strace: Fix crash caused over-optimization

Jon TURNEY
On 15/04/2017 23:27, Daniel Santos wrote:

> Recent versions of gcc are optimizing away the TLS buffer allocated in
> main, so we need to tell gcc that it's really used.
> ---
>  winsup/utils/strace.cc | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
> index beab67b90..1e581b4a4 100644
> --- a/winsup/utils/strace.cc
> +++ b/winsup/utils/strace.cc
> @@ -1192,6 +1192,8 @@ main (int argc, char **argv)
>    char buf[CYGTLS_PADSIZE];
>
>    memset (buf, 0, sizeof (buf));
> +  /* Prevent buf from being optimized away.  */
> +  __asm__ __volatile__("" :: "m" (buf));

wouldn't adding volatile to the definition of buf be a better way to
write this?

>    exit (main2 (argc, argv));
>  }
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] strace: Fix crash caused over-optimization

Daniel Santos
On 04/16/2017 05:21 AM, Jon Turney wrote:

> On 15/04/2017 23:27, Daniel Santos wrote:
>> Recent versions of gcc are optimizing away the TLS buffer allocated in
>> main, so we need to tell gcc that it's really used.
>> ---
>>  winsup/utils/strace.cc | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
>> index beab67b90..1e581b4a4 100644
>> --- a/winsup/utils/strace.cc
>> +++ b/winsup/utils/strace.cc
>> @@ -1192,6 +1192,8 @@ main (int argc, char **argv)
>>    char buf[CYGTLS_PADSIZE];
>>
>>    memset (buf, 0, sizeof (buf));
>> +  /* Prevent buf from being optimized away.  */
>> +  __asm__ __volatile__("" :: "m" (buf));
>
> wouldn't adding volatile to the definition of buf be a better way to
> write this?

I actually did try that, although I had guessed it wouldn't (and
shouldn't) work.  I believe that the reason is that rather the accesses
are volatile or not, gcc can see nothing else using it and memset can be
a treated as a compiler built-in (per the C spec, maybe C89?), so it can
presume the outcome.  If there's a cleaner way to do this, I would
really love to learn that.  __attribute__ ((used)) only works on
variables with static storage.

Now I suspect that I may have found a little bug in gcc, because if I
call memset by casting it directly as a volatile function pointer, it is
still optimized away, and it should not:

   ((void *(*volatile)(void *, int, size_t))memset) (buf, 0, sizeof (buf));

And most interestingly, if I first assign a local volatile function
pointer to the address, then gcc properly does *not* optimize it away:

   void *(*volatile vol_memset)(void *, int, size_t) = memset;
   vol_memset (buf, 0, sizeof (buf));

I'm actually really glad for your response and that I played with this
because I need to make sure that this problem doesn't exist in gcc7.  I
have changes going into gcc8 shortly and this could mask problems from
my test program where I cast functions as volatile w/o assigning using a
local.

Daniel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] strace: Fix crash caused over-optimization

Corinna Vinschen-2
On Apr 17 03:39, Daniel Santos wrote:

> On 04/16/2017 05:21 AM, Jon Turney wrote:
> > On 15/04/2017 23:27, Daniel Santos wrote:
> > > Recent versions of gcc are optimizing away the TLS buffer allocated in
> > > main, so we need to tell gcc that it's really used.
> > > ---
> > >  winsup/utils/strace.cc | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
> > > index beab67b90..1e581b4a4 100644
> > > --- a/winsup/utils/strace.cc
> > > +++ b/winsup/utils/strace.cc
> > > @@ -1192,6 +1192,8 @@ main (int argc, char **argv)
> > >    char buf[CYGTLS_PADSIZE];
> > >
> > >    memset (buf, 0, sizeof (buf));
> > > +  /* Prevent buf from being optimized away.  */
> > > +  __asm__ __volatile__("" :: "m" (buf));
> >
> > wouldn't adding volatile to the definition of buf be a better way to
> > write this?
>
> I actually did try that, although I had guessed it wouldn't (and shouldn't)
> work.  I believe that the reason is that rather the accesses are volatile or
> not, gcc can see nothing else using it and memset can be a treated as a
> compiler built-in (per the C spec, maybe C89?), so it can presume the
> outcome.  If there's a cleaner way to do this, I would really love to learn
> that.  __attribute__ ((used)) only works on variables with static storage.
>
> Now I suspect that I may have found a little bug in gcc, because if I call
> memset by casting it directly as a volatile function pointer, it is still
> optimized away, and it should not:
>
>   ((void *(*volatile)(void *, int, size_t))memset) (buf, 0, sizeof (buf));
>
> And most interestingly, if I first assign a local volatile function pointer
> to the address, then gcc properly does *not* optimize it away:
>
>   void *(*volatile vol_memset)(void *, int, size_t) = memset;
>   vol_memset (buf, 0, sizeof (buf));
>
> I'm actually really glad for your response and that I played with this
> because I need to make sure that this problem doesn't exist in gcc7.  I have
> changes going into gcc8 shortly and this could mask problems from my test
> program where I cast functions as volatile w/o assigning using a local.
>
> Daniel
What about using RtlSecureZeroMemory instead?


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] strace: Fix crash caused over-optimization

Daniel Santos
On 04/18/2017 05:04 AM, Corinna Vinschen wrote:

> On Apr 17 03:39, Daniel Santos wrote:
>
>> I actually did try that, although I had guessed it wouldn't (and shouldn't)
>> work.  I believe that the reason is that rather the accesses are volatile or
>> not, gcc can see nothing else using it and memset can be a treated as a
>> compiler built-in (per the C spec, maybe C89?), so it can presume the
>> outcome.  If there's a cleaner way to do this, I would really love to learn
>> that.  __attribute__ ((used)) only works on variables with static storage.
>>
>> Now I suspect that I may have found a little bug in gcc, because if I call
>> memset by casting it directly as a volatile function pointer, it is still
>> optimized away, and it should not:
>>
>>    ((void *(*volatile)(void *, int, size_t))memset) (buf, 0, sizeof (buf));
>>
>> And most interestingly, if I first assign a local volatile function pointer
>> to the address, then gcc properly does *not* optimize it away:
>>
>>    void *(*volatile vol_memset)(void *, int, size_t) = memset;
>>    vol_memset (buf, 0, sizeof (buf));
>>
>> I'm actually really glad for your response and that I played with this
>> because I need to make sure that this problem doesn't exist in gcc7.  I have
>> changes going into gcc8 shortly and this could mask problems from my test
>> program where I cast functions as volatile w/o assigning using a local.
>>
>> Daniel
> What about using RtlSecureZeroMemory instead?
>
>
> Corinna

Well that's surprising.  Yes, it does solve the problem and I presume
would be more portable. :)  It even inlines the "memset", but uses a
single byte rep stos.  Technically, I think that a double-word stos
could be used in this case, but I doubt that matters much.

Daniel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2] strace: Fix "over-optimization" flaw in strace.

Daniel Santos
In reply to this post by Corinna Vinschen-2
Recent versions of gcc are optimizing away the TLS buffer allocated in
main, so we need to tell gcc that it's really used.  RtlSecureZeroMemory
accomplishes this while also inlining the memset.

Signed-off-by: Daniel Santos <[hidden email]>
---
 winsup/utils/strace.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
index beab67b90..ae62cdc5f 100644
--- a/winsup/utils/strace.cc
+++ b/winsup/utils/strace.cc
@@ -1191,7 +1191,7 @@ main (int argc, char **argv)
      registry setting to 0x100000 (TOP_DOWN). */
   char buf[CYGTLS_PADSIZE];
 
-  memset (buf, 0, sizeof (buf));
+  RtlSecureZeroMemory (buf, sizeof (buf));
   exit (main2 (argc, argv));
 }
 
--
2.11.0

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

resend: Re: [PATCH] strace: Fix crash caused over-optimization

Daniel Santos
In reply to this post by Daniel Santos
sourceware.org decided that I was a spammer for some weird reason...
Maybe this one will go through...

On 04/19/2017 10:52 AM, [hidden email] wrote:

> Hi. This is the qmail-send program at sourceware.org.
> I'm afraid I wasn't able to deliver your message to the following addresses.
> This is a permanent error; I've given up. Sorry it didn't work out.
>
> <[hidden email]>:
> Mail rejected: List address must be in To: or Cc: headers.
> See http://sourceware.org/lists.html#sourceware-list-info for more information.
>      
> If you are not a "spammer", we apologize for the inconvenience.
> You can add yourself to the cygwin.com "global allow list"
> by sending email *from*the*blocked*email*address* to:
>    


-------- Forwarded Message --------
Subject: Re: [PATCH] strace: Fix crash caused over-optimization
Date: Wed, 19 Apr 2017 10:57:02 -0500
From: Daniel Santos <[hidden email]>
To: [hidden email], Corinna Vinschen
<[hidden email]>



On 04/18/2017 05:04 AM, Corinna Vinschen wrote:

> On Apr 17 03:39, Daniel Santos wrote:
>
>> I actually did try that, although I had guessed it wouldn't (and shouldn't)
>> work.  I believe that the reason is that rather the accesses are volatile or
>> not, gcc can see nothing else using it and memset can be a treated as a
>> compiler built-in (per the C spec, maybe C89?), so it can presume the
>> outcome.  If there's a cleaner way to do this, I would really love to learn
>> that.  __attribute__ ((used)) only works on variables with static storage.
>>
>> Now I suspect that I may have found a little bug in gcc, because if I call
>> memset by casting it directly as a volatile function pointer, it is still
>> optimized away, and it should not:
>>
>>    ((void *(*volatile)(void *, int, size_t))memset) (buf, 0, sizeof (buf));
>>
>> And most interestingly, if I first assign a local volatile function pointer
>> to the address, then gcc properly does *not* optimize it away:
>>
>>    void *(*volatile vol_memset)(void *, int, size_t) = memset;
>>    vol_memset (buf, 0, sizeof (buf));
>>
>> I'm actually really glad for your response and that I played with this
>> because I need to make sure that this problem doesn't exist in gcc7.  I have
>> changes going into gcc8 shortly and this could mask problems from my test
>> program where I cast functions as volatile w/o assigning using a local.
>>
>> Daniel
> What about using RtlSecureZeroMemory instead?
>
>
> Corinna

Well that's surprising.  Yes, it does solve the problem and I presume
would be more portable. :)  It even inlines the "memset", but uses a
single byte rep stos.  Technically, I think that a double-word stos
could be used in this case, but I doubt that matters much.

Daniel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] strace: Fix "over-optimization" flaw in strace.

Corinna Vinschen-2
In reply to this post by Daniel Santos
On Apr 19 11:06, Daniel Santos wrote:

> Recent versions of gcc are optimizing away the TLS buffer allocated in
> main, so we need to tell gcc that it's really used.  RtlSecureZeroMemory
> accomplishes this while also inlining the memset.
>
> Signed-off-by: Daniel Santos <[hidden email]>
> ---
>  winsup/utils/strace.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
> index beab67b90..ae62cdc5f 100644
> --- a/winsup/utils/strace.cc
> +++ b/winsup/utils/strace.cc
> @@ -1191,7 +1191,7 @@ main (int argc, char **argv)
>       registry setting to 0x100000 (TOP_DOWN). */
>    char buf[CYGTLS_PADSIZE];
>  
> -  memset (buf, 0, sizeof (buf));
> +  RtlSecureZeroMemory (buf, sizeof (buf));
>    exit (main2 (argc, argv));
>  }
>  
> --
> 2.11.0
Pushed.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] strace: Fix "over-optimization" flaw in strace.

Corinna Vinschen-2
On Apr 19 17:24, Daniel Santos wrote:
> On 04/19/2017 05:10 PM, Daniel Santos wrote:
> > Thanks. I hope this message goes through. Earlier when I tried to
> > respond with both you and cygwin-patches in the To: header it bounced. I
> > emailed [hidden email] about this, is that the right address
> > for mailing list problems?
>
> Actually, any email I send with [hidden email] in the To header
> is bounced.

Yes, it's a write-only mail address.  Please send stuff only to the
respective mailing list.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] strace: Fix "over-optimization" flaw in strace.

Daniel Santos
On 04/20/2017 03:45 AM, Corinna Vinschen wrote:
> Yes, it's a write-only mail address.  Please send stuff only to the
> respective mailing list.

Oh, I see. Thanks for the clarification.

Daniel
Loading...