| Message ID | CAGQ9bdxLE9R_X2V8z+Gu7dzzC+yrgg5pfYNUk+C2BY9JKKVu=Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 22, 2014 at 06:34:22PM +0400, Konstantin Serebryany wrote: > > Notes: > > 1) the cases where we e.g. for various stringops perform first and > > last address check and use separate __asan_report_*1 for reporting > > that should probably be converted to use this __asan_report_*_n too > > Note that in clang we completely removed the code that instruments > srtingops (we had memset/memcpy/memmove). > Instead we replace memset with __asan_memset (ditto for > memcpy/memmove) so that it does not get inlined later. > This simplifies the code and allows to properly analyze all memset/etc > calls, not just check the first and the last bytes. Food for thought, yes. > > 3) there is still a failure for -m32: > > FAIL: g++.dg/asan/asan_test.C -O2 AddressSanitizer_UAF_long_double Ident(p)[12] = 0 output pattern test > > Output should match: WRITE of size 1[06] > > FAIL: g++.dg/asan/asan_test.C -O2 AddressSanitizer_UAF_long_double Ident(p)[0] = Ident(p)[12] output pattern test > > Output should match: READ of size 1[06] > > That sounds like something to fix in upstream, it should allow also size > > 12 which is the size of long double on ia32 (16 bytes on x86_64), > > thus 1[026]. Kostya, can you please change it, I'll then apply it > > to the testsuite patch too? > Like this? > > --- lib/asan/tests/asan_test.cc (revision 209430) > +++ lib/asan/tests/asan_test.cc (working copy) > @@ -183,8 +183,8 @@ > TEST(AddressSanitizer, UAF_long_double) { > if (sizeof(long double) == sizeof(double)) return; > long double *p = Ident(new long double[10]); > - EXPECT_DEATH(Ident(p)[12] = 0, "WRITE of size 1[06]"); > - EXPECT_DEATH(Ident(p)[0] = Ident(p)[12], "READ of size 1[06]"); > + EXPECT_DEATH(Ident(p)[12] = 0, "WRITE of size 1[026]"); > + EXPECT_DEATH(Ident(p)[0] = Ident(p)[12], "READ of size 1[026]"); > delete [] Ident(p); > } > Yep, exactly. > > As mentioned earlier, ubsan has similar > > problem where it doesn't recognize float bitsize 96 (but unlike this > > case where clang+llvm pass in 10 bytes, which is what is actually > > accessed by hw if using i387 stack, but not if using other means of > > copying it, in ubsan case clang also passes in bitsize 96 that ubsan > > doesn't handle). > > Yea, this long double business is rather confusing to me... It is actually even more complicated than that, on x86_64 GCC supports long double (== __float80) which has 128 bits (64-bit) or 96 bits (32-bit), and is the 80-bit extended double format, plus also __float128, which is a software implemented IEEE 754 quad long double (also 128 bit). Now, ubsan handles all of 80 and 128 bit (and should 96 bit) floats as long double, so it is unclear how to tell libubsan about __float128 type (because bitsize 128 is already used for something else), plus in order to actually print __float128 one would need to link in libquadmath, which is probably not desirable. So, ideally there should be some way to tell the library we have IEEE 754 quad __float128 and for now the library should just print that the number is not printable. Ditto for _Decimal{32,64,128}, which should probably just be a different kind from float, and again would require additional library to support printing into strings, so perhaps it could be for now just printed as something unprintable. Jakub
> >> > 3) there is still a failure for -m32: >> > FAIL: g++.dg/asan/asan_test.C -O2 AddressSanitizer_UAF_long_double Ident(p)[12] = 0 output pattern test >> > Output should match: WRITE of size 1[06] >> > FAIL: g++.dg/asan/asan_test.C -O2 AddressSanitizer_UAF_long_double Ident(p)[0] = Ident(p)[12] output pattern test >> > Output should match: READ of size 1[06] >> > That sounds like something to fix in upstream, it should allow also size >> > 12 which is the size of long double on ia32 (16 bytes on x86_64), >> > thus 1[026]. Kostya, can you please change it, I'll then apply it >> > to the testsuite patch too? >> Like this? >> >> --- lib/asan/tests/asan_test.cc (revision 209430) >> +++ lib/asan/tests/asan_test.cc (working copy) >> @@ -183,8 +183,8 @@ >> TEST(AddressSanitizer, UAF_long_double) { >> if (sizeof(long double) == sizeof(double)) return; >> long double *p = Ident(new long double[10]); >> - EXPECT_DEATH(Ident(p)[12] = 0, "WRITE of size 1[06]"); >> - EXPECT_DEATH(Ident(p)[0] = Ident(p)[12], "READ of size 1[06]"); >> + EXPECT_DEATH(Ident(p)[12] = 0, "WRITE of size 1[026]"); >> + EXPECT_DEATH(Ident(p)[0] = Ident(p)[12], "READ of size 1[026]"); >> delete [] Ident(p); >> } >> > > Yep, exactly. done, r209445.
>> 2) it doesn't still deal with unaligned power of two accesses properly, >> but neither does llvm (at least not 3.4). Am not talking about >> undefined behavior cases where the compiler isn't told the access >> is misaligned, but e.g. when accessing struct S { int x; } >> __attribute__((packed)) and similar (or aligned(1)). Supposedly >> we could force __asan_report_*_n for that case too, because >> normal wider check assumes it is aligned > > Yep, we don't do it. Now we do: http://llvm.org/viewvc/llvm-project?rev=209508&view=rev
--- lib/asan/tests/asan_test.cc (revision 209430) +++ lib/asan/tests/asan_test.cc (working copy) @@ -183,8 +183,8 @@ TEST(AddressSanitizer, UAF_long_double) { if (sizeof(long double) == sizeof(double)) return; long double *p = Ident(new long double[10]); - EXPECT_DEATH(Ident(p)[12] = 0, "WRITE of size 1[06]"); - EXPECT_DEATH(Ident(p)[0] = Ident(p)[12], "READ of size 1[06]"); + EXPECT_DEATH(Ident(p)[12] = 0, "WRITE of size 1[026]"); + EXPECT_DEATH(Ident(p)[0] = Ident(p)[12], "READ of size 1[026]"); delete [] Ident(p); }