[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|REOPENED|RESOLVED
 Resolution|--- |FIXED

--- Comment #18 from Martin Liška  ---
Should be fixed on trunk.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

--- Comment #17 from Martin Liška  ---
Author: marxin
Date: Tue Jul  4 10:53:18 2017
New Revision: 249960

URL: https://gcc.gnu.org/viewcvs?rev=249960=gcc=rev
Log:
Enable addressable params sanitization with --param asan-stack=1.

2017-07-04  Martin Liska  

PR sanitizer/81040
* sanopt.c (sanitize_rewrite_addressable_params): Mark the
newly created variable as DECL_IGNORED_P.
2017-07-04  Martin Liska  

PR sanitizer/81040
* g++.dg/asan/function-argument-1.C: Run the test-case w/o
use-after-scope sanitization.

Modified:
trunk/gcc/ChangeLog
trunk/gcc/sanopt.c
trunk/gcc/testsuite/ChangeLog
trunk/gcc/testsuite/g++.dg/asan/function-argument-1.C

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|RESOLVED|REOPENED
 Resolution|FIXED   |---

--- Comment #16 from Martin Liška  ---
(In reply to Andrey Ryabinin from comment #15)
> (In reply to Martin Liška from comment #14)
> > Fixed on trunk.
> 
> Thanks.
> 
> However there is slight problem with this. Instrumentation is missing without
> -fsanitize-address-use-after-scope option.
> IMO, it should depend only on --param asan-stack=1.
> 
> So the following doesn't produce any output:
> gcc -fsanitize=address -fno-sanitize-address-use-after-scope -O2 asan_test.c
> && ./a.out
> 
> Can we fix this?

It's mistake! I've got patch for that and will send it tomorrow.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[ryabinin.a.a at gmail dot com]

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

--- Comment #15 from Andrey Ryabinin  ---
(In reply to Martin Liška from comment #14)
> Fixed on trunk.

Thanks.

However there is slight problem with this. Instrumentation is missing without
-fsanitize-address-use-after-scope option.
IMO, it should depend only on --param asan-stack=1.

So the following doesn't produce any output:
gcc -fsanitize=address -fno-sanitize-address-use-after-scope -O2 asan_test.c &&
./a.out

Can we fix this?

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #14 from Martin Liška  ---
Fixed on trunk.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

--- Comment #13 from Martin Liška  ---
Author: marxin
Date: Mon Jul  3 11:48:47 2017
New Revision: 249903

URL: https://gcc.gnu.org/viewcvs?rev=249903=gcc=rev
Log:
ASAN: handle addressable params (PR sanitize/81040).

2017-07-03  Martin Liska  

PR sanitize/81040
* g++.dg/asan/function-argument-1.C: New test.
* g++.dg/asan/function-argument-2.C: New test.
* g++.dg/asan/function-argument-3.C: New test.
2017-07-03  Martin Liska  

PR sanitize/81040
* sanopt.c (rewrite_usage_of_param): New function.
(sanitize_rewrite_addressable_params): Likewise.
(pass_sanopt::execute): Call rewrite_usage_of_param.

Added:
trunk/gcc/testsuite/g++.dg/asan/function-argument-1.C
trunk/gcc/testsuite/g++.dg/asan/function-argument-2.C
trunk/gcc/testsuite/g++.dg/asan/function-argument-3.C
Modified:
trunk/gcc/ChangeLog
trunk/gcc/doc/extend.texi
trunk/gcc/sanopt.c
trunk/gcc/testsuite/ChangeLog

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

URL||https://github.com/google/s
   ||anitizers/issues/823

--- Comment #12 from Martin Liška  ---
Adding clang sanitizer issue where argument of type struct { int[5] } is not
handled.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED

--- Comment #11 from Martin Liška  ---
(In reply to Jakub Jelinek from comment #10)
> (In reply to Martin Liška from comment #9)
> > Thanks Jakub for your thoughts. I was also thinking about simple approach
> > similar to what we do for use-after-scope in situations where there's a
> > usage of IFN_ASAN_POISON. In that case we generate a 'shadow' variable
> > that's poison/unpoisoned, etc. We can come up with automatic variables to
> > which we assign an addressable PARM_DECL and then these variables will be
> > normally protected with red zones. I can imaging bigger performance gap, but
> > it can be quite easily doable I guess?
> 
> That would work too.  If doing it this way, it would be desirable to do it
> late in the GIMPLE pass queue, e.g. in the sanopt pass, for the
> TREE_ADDRESSABLE PARM_DECLs that don't have TREE_ADDRESSABLE types (those
> are passed by invisible reference) create the DECL_ARTIFICIAL VAR_DECLs,
> have them TREE_ADDRESSABLE, add
> code to copy the PARM_DECLs to those vars at the beginning of the function,
> clear TREE_ADDRESSABLE on the PARM_DECLs and remap all uses of the
> PARM_DECLs except the newly added ones to the new VAR_DECLs.  Not sure if
> the PARM_DECLs should have DECL_VALUE_EXPR pointing to the VAR_DECLs (e.g.
> for debug info purposes), and whether the artificial VAR_DECLs should be
> added to some BLOCK_VARS or not.

Good. I will implement that in this stage1 ;)

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek  ---
(In reply to Martin Liška from comment #9)
> Thanks Jakub for your thoughts. I was also thinking about simple approach
> similar to what we do for use-after-scope in situations where there's a
> usage of IFN_ASAN_POISON. In that case we generate a 'shadow' variable
> that's poison/unpoisoned, etc. We can come up with automatic variables to
> which we assign an addressable PARM_DECL and then these variables will be
> normally protected with red zones. I can imaging bigger performance gap, but
> it can be quite easily doable I guess?

That would work too.  If doing it this way, it would be desirable to do it late
in the GIMPLE pass queue, e.g. in the sanopt pass, for the TREE_ADDRESSABLE
PARM_DECLs that don't have TREE_ADDRESSABLE types (those are passed by
invisible reference) create the DECL_ARTIFICIAL VAR_DECLs, have them
TREE_ADDRESSABLE, add
code to copy the PARM_DECLs to those vars at the beginning of the function,
clear TREE_ADDRESSABLE on the PARM_DECLs and remap all uses of the PARM_DECLs
except the newly added ones to the new VAR_DECLs.  Not sure if the PARM_DECLs
should have DECL_VALUE_EXPR pointing to the VAR_DECLs (e.g. for debug info
purposes), and whether the artificial VAR_DECLs should be added to some
BLOCK_VARS or not.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

--- Comment #9 from Martin Liška  ---
Thanks Jakub for your thoughts. I was also thinking about simple approach
similar to what we do for use-after-scope in situations where there's a usage
of IFN_ASAN_POISON. In that case we generate a 'shadow' variable that's
poison/unpoisoned, etc. We can come up with automatic variables to which we
assign an addressable PARM_DECL and then these variables will be normally
protected with red zones. I can imaging bigger performance gap, but it can be
quite easily doable I guess?

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[jakub at gcc dot gnu.org]

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

Jakub Jelinek  changed:

   What|Removed |Added

 Status|WAITING |NEW

--- Comment #8 from Jakub Jelinek  ---
The problem is that on the stack we just instrument the user variables (those
seen in expand_used_vars).  But that is not the case of addressable arguments,
those aren't known to need some stack slot until later, in particular the
pass_expand::execute method after expand_used_vars calls expand_function_start
and that calls assign_parms -> assign_parm_setup_stack -> assign_stack_local.
Now, some addressable arguments that are passed on the stack actually are using
the argument passing slot where you can't insert any red zones around, it is
part of ABI how arguments are passed, but in other cases (e.g. if the argument
is not passed on the stack but e.g. in registers, or partially in
registers/partially on the stack, or fully on the stack, but with smaller
guaranteed alignment) the compiler copies those arguments from those spots to
other stack slots and those slots could be in theory using red zones.
It is hard though, because for other vars we are conditionally using asan
allocation instead of stack slots (decided at runtime).
So, in order to instrument addressable arguments, we'd need to guess in
expand_used_vars for which arguments we'll need an asan protected slot
(PARM_DECLs that are TREE_ADDRESSABLE, but their types aren't TREE_ADDRESSABLE)
and include those into the allocations and remember it somewhere (not sure if
DECL_RTL on PARM_DECL is suitable for it or not), and then later on arrange for
those slots to be what is used instead of assign_stack_local.
And then there is another problem, because while the asan allocation/shadow
initialization sequence is created earlier (expand_used_vars), it is actually
emitted later (after the copying of the parameters into stack slots if
necessary).  That order has a good reason, e.g. __asan_stack_malloc_* function
is a normal function call, so clobbers registers used for argument passing.
So perhaps in some worst cases we'd need to copy the arguments into a temporary
slot and then again copy them into the asan red zone protected one.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[ryabinin.a.a at gmail dot com]

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

--- Comment #7 from Andrey Ryabinin  ---
(In reply to Martin Liška from comment #6)
> Can't see that clang++ is capable of catching your first test-case:
> 
> $ clang++ pr81040.cpp -fsanitize=address && ./a.out 
> pr81040.cpp:3:9: warning: expression result unused; assign into a variable
> to force a volatile load [-Wunused-volatile-lvalue]
> *(volatile int*)a;
> ^
> 1 warning generated.
> 

Try simply 'clang' without ++ 
Seems clang++ just optimize away unused volatile lvalue.

Or just make goo() to use *a, e.g.:
static __attribute__((noinline)) int goo(int *a)
{   
return *(volatile int*)a;   
}


> $ clang++ --version
> clang version 4.0.0 (tags/RELEASE_400/final 297347)
> Target: x86_64-unknown-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|NEW |WAITING

--- Comment #6 from Martin Liška  ---
Can't see that clang++ is capable of catching your first test-case:

$ clang++ pr81040.cpp -fsanitize=address && ./a.out 
pr81040.cpp:3:9: warning: expression result unused; assign into a variable to
force a volatile load [-Wunused-volatile-lvalue]
*(volatile int*)a;
^
1 warning generated.

$ clang++ --version
clang version 4.0.0 (tags/RELEASE_400/final 297347)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|RESOLVED|NEW
 Resolution|WONTFIX |---
   Severity|normal  |enhancement

--- Comment #5 from Martin Liška  ---
(In reply to Andrey Ryabinin from comment #4)
> (In reply to Martin Liška from comment #3)
> > As mentioned by Richard, currently ASAN is able to protect function
> > variables that live on stack. In your case the function foo is called with
> > constant that is then assigned a stack slot which we don't instrument with
> > red zones.
> 
> Exactly, that's the problem. That stack slot should be instrumented with
> redzones. Surely compiler is capable of doing this. Clang does this.
> So I don't understand why you closed this as WONTFIX.

Sorry, I was not aware that it's actually functionality supported by Clang.
Thus it's enhancement on GCC's side.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[ryabinin.a.a at gmail dot com]

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

--- Comment #4 from Andrey Ryabinin  ---
(In reply to Martin Liška from comment #3)
> As mentioned by Richard, currently ASAN is able to protect function
> variables that live on stack. In your case the function foo is called with
> constant that is then assigned a stack slot which we don't instrument with
> red zones.

Exactly, that's the problem. That stack slot should be instrumented with
redzones. Surely compiler is capable of doing this. Clang does this.
So I don't understand why you closed this as WONTFIX.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |WONTFIX

--- Comment #3 from Martin Liška  ---
As mentioned by Richard, currently ASAN is able to protect function variables
that live on stack. In your case the function foo is called with constant that
is then assigned a stack slot which we don't instrument with red zones.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[marxin at gcc dot gnu.org]

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

Martin Liška  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2017-06-12
   Assignee|unassigned at gcc dot gnu.org  |marxin at gcc dot 
gnu.org
 Ever confirmed|0   |1

--- Comment #2 from Martin Liška  ---
(In reply to Richard Biener from comment #1)
> Likely instrumentation simply is missing
> 
>goo((int*));
> 
> given 'a' lives on the stack.  With static foo and IPA CP applied it
> eventually
> sees a address-taken constant pool reference.

Yes, with -O2 and static function, const prop creates clone of foo, where
variable 'a' lives on stack. Write in goo is checked, but it points to a valid
shadow memory (stack). I'm investigating more.

[Bug sanitizer/81040] asan false negative if parameter of a global function passed by reference

[rguenth at gcc dot gnu.org]

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

--- Comment #1 from Richard Biener  ---
Likely instrumentation simply is missing

   goo((int*));

given 'a' lives on the stack.  With static foo and IPA CP applied it eventually
sees a address-taken constant pool reference.