On Wed, Sep 26, 2018 at 11:33:25AM +0200, Martin Liška wrote:
> 2018-09-26 Martin Liska <mli...@suse.cz>
>
> PR sanitizer/81715
> * asan.c (asan_shadow_cst): Remove.
> (asan_emit_redzone_payload): New.
> (asan_emit_stack_protection): Make it more
> flexible to support arbitrary size of red zones.
> * asan.h (ASAN_MIN_RED_ZONE_SIZE): New.
> (asan_var_and_redzone_size): Likewise.
> * cfgexpand.c (expand_stack_vars): Reserve size
> for stack vars according to asan_var_and_redzone_size.
> (expand_used_vars): Make smaller offset based
> on ASAN_SHADOW_GRANULARITY.
>
> gcc/testsuite/ChangeLog:
>
> 2018-09-26 Martin Liska <mli...@suse.cz>
>
> PR sanitizer/81715
> * c-c++-common/asan/asan-stack-small.c: New test.
Sorry for the delay. I had a quick look. Using:
struct S { char a[32]; };
void bar (void *, void *, void *, void *);
int
foo (void)
{
struct S a, b, c, d;
bar (&a, &b, &c, &d);
return 0;
}
int
baz (void)
{
char a;
short b;
int c;
long long d;
bar (&a, &b, &c, &d);
return 0;
}
-O2 -fsanitize=address, I see that foo is identical before/after your patch,
perfect.
Looking at baz, I see issues though:
.LC2:
.string "4 32 1 4 a:15 48 2 4 b:16 64 4 4 c:17 80 8 4 d:18"
...
movq %rbx, %rbp
movl $-3580, %edx
movl $-3085, %ecx
movq $1102416563, (%rbx)
shrq $3, %rbp
movq $.LC2, 8(%rbx)
leaq 48(%rbx), %rsi
leaq 32(%rbx), %rdi
movq $.LASANPC1, 16(%rbx)
movw %dx, 2147450888(%rbp)
leaq 64(%rbx), %rdx
movw %cx, 2147450891(%rbp)
leaq 80(%rbx), %rcx
movl $-235802127, 2147450880(%rbp)
movl $-234687999, 2147450884(%rbp)
movb $-13, 2147450893(%rbp)
call bar
cmpq %rbx, %r12
jne .L15
movq $0, 2147450880(%rbp)
xorl %eax, %eax
movl $0, 2147450888(%rbp)
movw %ax, 2147450892(%rbp)
So, looks like the shadow at (%rbx>>3)+0x7fff8000 is:
/ 2147450880(%rbp)
| / 2147450884(%rbp)
| | / 2147450888(%rbp)
| | | / 2147450892(%rbp)
| | | |
f1 f1 f1 f1 01 f2 02 f2 04 f2 00 f3 f3 f3
Two problems, it uses unconditionally unaligned stores, without
checking if the target supports them at all (in this case it does).
And, it doesn't check if it wouldn't be more efficient to use
32-bit stores. It isn't that we want the gaps to have whatever
value the shadow memory contained before, we want it to be 0 and just rely
on it being zero from earlier.
Another question is if it wouldn't be worth to ensure the variable area is
always a multiple of 32 bytes (thus the corresponding shadow always multiple
of 4 bytes). Then, for this testcase, you'd store:
$-235802127, 2147450880(%rbp) // 0xf1f1f1f1
$-234687999, 2147450884(%rbp) // 0xf202f201
$-218041852, 2147450888(%rbp) // 0xf300f204
$-202116109, 2147450892(%rbp) // 0xf3f3f3f3
For the single char/short/int/long var in a function case you'd still emit
just f1 f1 f1 f1 0[1240] f3 f3 f3
and the number of final f3 bytes would be from 3 to 6 (or 2 to 5), 1 is
probably too few.
Rather than special-casing the two very small adjacent vars case,
just use a rolling handling of the shadow bytes, if you fill all 4,
emit immediately, otherwise queue later and flush if the next shadow offset
is outside of the 4 bytes. Either always use SImode stores, or check rtx
costs; if the 32-bit constants you'd store is as expensive or less than the
16-bit or 8-bit constant, use 32-bit store, otherwise use a 16-bit or 8-bit
one. If you want to complicate it further, allow unaligned stores on
targets that do allow them, but use them with care, if you could use same
amount of aligned stores with the same cost of constants, prefer aligned
ones.
Jakub
