Clang changes behaviour of MemIntrinsic functions before we instrument their arguments #4

Closed
ramosian-glider opened this Issue Aug 31, 2015 · 14 comments

Comments

Projects
None yet
2 participants
Member

ramosian-glider commented Aug 31, 2015

Originally reported on Google Code with ID 4

char * s = (char*)malloc(4096);
memset(s+4095, 0, 2);

Here Clang changes "memset 2 bytes" to a single "store i16" instruction, and we don't
instrument (and fail on) access to the memory outside allocated array.

Reported by samsonov@google.com on 2011-08-10 10:08:36

Member

ramosian-glider commented Aug 31, 2015

So, what is the problem? 
The code is buggy and asan detects the bug. 
No? 

Reported by konstantin.s.serebryany on 2011-08-10 10:16:04

Member

ramosian-glider commented Aug 31, 2015

The problem is with mops that "split 2 shadow bytes". Since we don't analyse the second
byte, we don't catch the error. The minimal test is:

TEST(AddressSanitizer, DISABLED_StrangeMemIntrinsicBehaviorTest2){
  int const size = 4096;
  char* s = (char*)malloc(size);
  EXPECT_DEATH(memcpy(s+size-1, s, 2), TO_THE_RIGHT(0));
  free(s);
}

If 4096 is replaced with 4095, the test passes (that is, the program crashes).

Reported by dvyukov@google.com on 2011-08-10 10:32:49

Member

ramosian-glider commented Aug 31, 2015

ah! 
This is http://code.google.com/p/address-sanitizer/wiki/AddressSanitizerAlgorithm#Unaligned_accesses

Not sure if we want to do anything with this right now... 

Reported by konstantin.s.serebryany on 2011-08-10 10:36:54

Member

ramosian-glider commented Aug 31, 2015

I think it's worth moving to KnownBugs, because it's where I looked first.

Reported by dvyukov@google.com on 2011-08-10 10:41:04

Member

ramosian-glider commented Aug 31, 2015

>> I think it's worth moving to KnownBugs,
Agree. Give a link there. 

Reported by konstantin.s.serebryany on 2011-08-10 10:53:37

Member

ramosian-glider commented Aug 31, 2015

Done:
http://code.google.com/p/address-sanitizer/wiki/KnownBugs

Reported by dvyukov@google.com on 2011-08-10 11:04:01

Member

ramosian-glider commented Aug 31, 2015

Can we instrument arguments of memintrinsic functions _before_ these functions are modified
by compiler and lead to unaligned access? Or we should just leave everything as is
now?

Reported by samsonov@google.com on 2011-08-10 11:19:15

Member

ramosian-glider commented Aug 31, 2015

I don't know for sure (need to investigate when does memset lowering happen), but probably
not. asan instrumentation should happen at the later stages, when the majority of other
optimizations already happened. 

I'd leave it as is for now. 
Long term we'll need to implement checking for unaligned accesses (as an option)

Reported by konstantin.s.serebryany on 2011-08-10 11:31:04

Member

ramosian-glider commented Aug 31, 2015

> Can we instrument arguments of memintrinsic functions _before_ these functions are
modified by compiler and lead to unaligned access? 

There should a compiler option that prevents inlining of intrinsic functions.

Reported by dvyukov@google.com on 2011-08-10 11:58:30

Member

ramosian-glider commented Aug 31, 2015

> There should a compiler option that prevents inlining of intrinsic functions

When I compile the following test with -fno-builtin

TEST(AddressSanitizer, DISABLED_StrangeMemIntrinsicBehaviorTest2){
  char * s = (char*)malloc(4096);
  memcpy(s+4096-1, s, 2);
}

it does not insert any instrumentation at all:

0808fb00 <AddressSanitizer_DISABLED_StrangeMemIntrinsicBehaviorTest2_Test::TestBody()>:
 808fb00:       55                      push   %ebp
 808fb01:       89 e5                   mov    %esp,%ebp
 808fb03:       83 ec 18                sub    $0x18,%esp
 808fb06:       c7 04 24 00 10 00 00    movl   $0x1000,(%esp)
 808fb0d:       e8 8e ea 09 00          call   812e5a0 <malloc>
 808fb12:       89 44 24 04             mov    %eax,0x4(%esp)
 808fb16:       05 ff 0f 00 00          add    $0xfff,%eax
 808fb1b:       89 04 24                mov    %eax,(%esp)
 808fb1e:       c7 44 24 08 02 00 00    movl   $0x2,0x8(%esp)
 808fb25:       00 
 808fb26:       e8 2d ea fe ff          call   807e558 <memcpy@plt>
 808fb2b:       83 c4 18                add    $0x18,%esp
 808fb2e:       5d                      pop    %ebp
 808fb2f:       c3                      ret    

ouch!

Reported by dvyukov@google.com on 2011-08-10 13:01:07

Member

ramosian-glider commented Aug 31, 2015

perhaps because it does not treat memset as an intrinsic

Reported by konstantin.s.serebryany on 2011-08-10 13:02:10

Member

ramosian-glider commented Aug 31, 2015

Yeah, but it should treat memcpy as a, well, memcpy.

Reported by dvyukov@google.com on 2011-08-10 13:14:32

Member

ramosian-glider commented Aug 31, 2015

since http://llvm.org/viewvc/llvm-project?rev=206746&view=rev
asan does not instrument memset/memmove/memcpy calls, instead it replaces the calls
with calls to __asan_memset/etc. 

I think this allows us to close this bug. 

Reported by konstantin.s.serebryany on 2014-05-14 13:44:00

  • Status changed: Fixed
Member

ramosian-glider commented Aug 31, 2015

Adding Project:AddressSanitizer as part of GitHub migration.

Reported by ramosian.glider on 2015-07-30 09:12:57

  • Labels added: ProjectAddressSanitizer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment