diff mbox

[asan] a small patch to fix bogus error about global buffer overflow

Message ID CA+4CFy4F4_EMNb8+wmMjRUaA-0Mhmy9=0C1xH-s4z-vyasbbKA@mail.gmail.com
State New
Headers show

Commit Message

Wei Mi Oct. 25, 2012, 9:32 p.m. UTC 
Hi,

A small patch to remove the bogus error reports exposed in the
spec2000 testing. In varasm.c, asan_protected should be equivalent
with asan_protect_global (decl) all the time, or else compiler will
not insert redzones for some globals planned to be protected.

gcc/ChangeLog:
2012-10-25   Wei Mi  <wmi@google.com>

A small fix to remove bogus error report of global buffer overflow.
* varasm.c: correct the condition of asan_protected being true.

   set_mem_align (decl_rtl, DECL_ALIGN (decl));

Thanks,
Wei.

Comments

Xinliang David Li Oct. 25, 2012, 9:45 p.m. UTC | #1 
Should the alignment check be moved into 'asan_protect_global' method?

David


On Thu, Oct 25, 2012 at 2:32 PM, Wei Mi <wmi@google.com> wrote:
> Hi,
>
> A small patch to remove the bogus error reports exposed in the
> spec2000 testing. In varasm.c, asan_protected should be equivalent
> with asan_protect_global (decl) all the time, or else compiler will
> not insert redzones for some globals planned to be protected.
>
> gcc/ChangeLog:
> 2012-10-25   Wei Mi  <wmi@google.com>
>
> A small fix to remove bogus error report of global buffer overflow.
> * varasm.c: correct the condition of asan_protected being true.
>
> Index: varasm.c
> ===================================================================
> --- varasm.c    (revision 192822)
> +++ varasm.c    (working copy)
> @@ -1991,11 +1991,10 @@ assemble_variable (tree decl, int top_le
>    align_variable (decl, dont_output_data);
>
>    if (flag_asan
> -      && asan_protect_global (decl)
> -      && DECL_ALIGN (decl) < ASAN_RED_ZONE_SIZE * BITS_PER_UNIT)
> +      && asan_protect_global (decl))
>      {
>        asan_protected = true;
> -      DECL_ALIGN (decl) = ASAN_RED_ZONE_SIZE * BITS_PER_UNIT;
> +      DECL_ALIGN (decl) = MAX (DECL_ALIGN (decl), ASAN_RED_ZONE_SIZE
> * BITS_PER_UNIT);
>      }
>
>    set_mem_align (decl_rtl, DECL_ALIGN (decl));
>
> Thanks,
> Wei.
Diego Novillo Oct. 25, 2012, 9:46 p.m. UTC | #2 
On Thu, Oct 25, 2012 at 5:32 PM, Wei Mi <wmi@google.com> wrote:
> Hi,
>
> A small patch to remove the bogus error reports exposed in the
> spec2000 testing. In varasm.c, asan_protected should be equivalent
> with asan_protect_global (decl) all the time, or else compiler will
> not insert redzones for some globals planned to be protected.
>
> gcc/ChangeLog:
> 2012-10-25   Wei Mi  <wmi@google.com>
>
> A small fix to remove bogus error report of global buffer overflow.

No need to put this in the ChangeLog entry.

> * varasm.c: correct the condition of asan_protected being true.

Tab before '*'.  Also, mention the function in varasm.c that was
changed.  So, it would look something like:

        * varasm.c (assemble_variable): correct the condition under
which asan_protected is true.

(yes, ChangeLog entries have silly requirements and inspire strong
feelings on both sides of the discussion.  The day we get rid of them,
I will weep with joy.).

The change looks fine to me, but why not just move the alignment check
into asan_protect_global?  I'll defer to David or Jakub in this.


Diego.
Jakub Jelinek Oct. 25, 2012, 9:46 p.m. UTC | #3 
On Thu, Oct 25, 2012 at 02:32:33PM -0700, Wei Mi wrote:
> A small patch to remove the bogus error reports exposed in the
> spec2000 testing. In varasm.c, asan_protected should be equivalent
> with asan_protect_global (decl) all the time, or else compiler will
> not insert redzones for some globals planned to be protected.
> 
> gcc/ChangeLog:
> 2012-10-25   Wei Mi  <wmi@google.com>
> 
> A small fix to remove bogus error report of global buffer overflow.
> * varasm.c: correct the condition of asan_protected being true.

The patch is almost ok, the ChangeLog entry is not.
Two instead of 3 spaces between date and name, no need for introductory
comment above * varasm.c line, name of modified function and capital
letter after :.
Perhaps
	* varasm.c (assemble_variable): Set asan_protected even for decls
	that are already ASAN_RED_ZONE_SIZE or more bytes aligned.

Ok with that Change and:

> --- varasm.c    (revision 192822)
> +++ varasm.c    (working copy)
> @@ -1991,11 +1991,10 @@ assemble_variable (tree decl, int top_le
>    align_variable (decl, dont_output_data);
> 
>    if (flag_asan
> -      && asan_protect_global (decl)
> -      && DECL_ALIGN (decl) < ASAN_RED_ZONE_SIZE * BITS_PER_UNIT)
> +      && asan_protect_global (decl))
>      {
>        asan_protected = true;
> -      DECL_ALIGN (decl) = ASAN_RED_ZONE_SIZE * BITS_PER_UNIT;
> +      DECL_ALIGN (decl) = MAX (DECL_ALIGN (decl), ASAN_RED_ZONE_SIZE
 * BITS_PER_UNIT);

Too long line, put ASAN_RED_ZONE_SIZE * BITS_PER_UNIT on next
line below the second DECL_ALIGN (decl).

	Jakub
Jakub Jelinek Oct. 25, 2012, 9:51 p.m. UTC | #4 
On Thu, Oct 25, 2012 at 05:46:47PM -0400, Diego Novillo wrote:
> The change looks fine to me, but why not just move the alignment check
> into asan_protect_global?  I'll defer to David or Jakub in this.

asan_protect_global has
|| DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE
check among other things (to prevent adding guards say between
a series of 64KB aligned vars where even inserting 32 bytes
in between would make the section 64KB larger).
It is just fine to protect 32-byte aligned variables (or
64-byte aligned ones), on the varasm.c side it was just a thinko,
one thing was that for protected globals we want to make sure the
alignment is at least 32-byte and another is that I wanted to avoid another
asan_protect_global call for whether to put there padding or not.

	Jakub
Xinliang David Li Oct. 25, 2012, 9:56 p.m. UTC | #5 
Why not relaxing the check even more to allow for instance 128 byte
alignment which may be common?

David

On Thu, Oct 25, 2012 at 2:51 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 25, 2012 at 05:46:47PM -0400, Diego Novillo wrote:
>> The change looks fine to me, but why not just move the alignment check
>> into asan_protect_global?  I'll defer to David or Jakub in this.
>
> asan_protect_global has
> || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE
> check among other things (to prevent adding guards say between
> a series of 64KB aligned vars where even inserting 32 bytes
> in between would make the section 64KB larger).
> It is just fine to protect 32-byte aligned variables (or
> 64-byte aligned ones), on the varasm.c side it was just a thinko,
> one thing was that for protected globals we want to make sure the
> alignment is at least 32-byte and another is that I wanted to avoid another
> asan_protect_global call for whether to put there padding or not.
>
>         Jakub
diff mbox

Patch

Index: varasm.c
===================================================================
--- varasm.c    (revision 192822)
+++ varasm.c    (working copy)
@@ -1991,11 +1991,10 @@  assemble_variable (tree decl, int top_le
   align_variable (decl, dont_output_data);

   if (flag_asan
-      && asan_protect_global (decl)
-      && DECL_ALIGN (decl) < ASAN_RED_ZONE_SIZE * BITS_PER_UNIT)
+      && asan_protect_global (decl))
     {
       asan_protected = true;
-      DECL_ALIGN (decl) = ASAN_RED_ZONE_SIZE * BITS_PER_UNIT;
+      DECL_ALIGN (decl) = MAX (DECL_ALIGN (decl), ASAN_RED_ZONE_SIZE
* BITS_PER_UNIT);
     }