Issue 6308048: [Sanitizer] Use ProcessMaps in symbolizer to get module name and offset. (Closed)
|
Created:
12 years, 8 months ago by samsonov Modified:
12 years, 7 months ago Base URL:
https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/ Visibility:
Public. |
Patch Set 1 : z #
Total comments: 14
Patch Set 2 : z #Patch Set 3 : z #
Total comments: 11
Patch Set 4 : z #
Total comments: 2
Patch Set 5 : z #Patch Set 6 : z #
MessagesTotal messages: 16
http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... File sanitizer_common/sanitizer_symbolizer.cc (right): http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:26: // FIXME: use internal_memset so, just do it. http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:63: // FIXME: use internal_strrchr same here http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:120: ~Symbolizer(); what is this for? http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:124: static Symbolizer *symbolizer; can't it be a global object with Init() member? This will avoid the need to InternalALloc call http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:135: AddressInfoList *list = (AddressInfoList*)InternalAlloc( on a side note, I don't like a linked list here. Can we change the interface to pass a result array and it's size instead of returning linked list? http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... File sanitizer_common/sanitizer_symbolizer.h (right): http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.h:56: // This function is (currently) NOT thread-safe. what does this mean?
Sign in to reply to this message.
http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... File sanitizer_common/sanitizer_symbolizer.cc (right): http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:26: // FIXME: use internal_memset On 2012/06/08 12:28:07, kcc1 wrote: > so, just do it. Done. http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:63: // FIXME: use internal_strrchr On 2012/06/08 12:28:07, kcc1 wrote: > same here Done. http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:120: ~Symbolizer(); On 2012/06/08 12:28:07, kcc1 wrote: > what is this for? Removed. http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:124: static Symbolizer *symbolizer; On 2012/06/08 12:28:07, kcc1 wrote: > can't it be a global object with Init() member? > This will avoid the need to InternalALloc call sanitizer_common/ is a part of TSan RTL, and global ctors are forbidden there (so the code just won't compile). http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:135: AddressInfoList *list = (AddressInfoList*)InternalAlloc( On 2012/06/08 12:28:07, kcc1 wrote: > on a side note, I don't like a linked list here. > Can we change the interface to pass a result array and it's size instead of > returning linked list? Done. http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... File sanitizer_common/sanitizer_symbolizer.h (right): http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.h:56: // This function is (currently) NOT thread-safe. On 2012/06/08 12:28:07, kcc1 wrote: > what does this mean? Two threads shouldn't call SymbolizeCode(...) simultaneously.
Sign in to reply to this message.
http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... File sanitizer_common/sanitizer_symbolizer.cc (right): http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.cc:124: static Symbolizer *symbolizer; I am not asking you to use a global with a CTOR, which we indeed ban. I ask you to create a linker initialized POD object which will have an Init method. On 2012/06/09 10:41:48, samsonov wrote: > On 2012/06/08 12:28:07, kcc1 wrote: > > can't it be a global object with Init() member? > > This will avoid the need to InternalALloc call > > sanitizer_common/ is a part of TSan RTL, and global ctors are forbidden there > (so the code just won't compile). http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... File sanitizer_common/sanitizer_symbolizer.h (right): http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... sanitizer_common/sanitizer_symbolizer.h:56: // This function is (currently) NOT thread-safe. So, why don't you write exactly that? On 2012/06/09 10:41:48, samsonov wrote: > On 2012/06/08 12:28:07, kcc1 wrote: > > what does this mean? > > Two threads shouldn't call SymbolizeCode(...) simultaneously.
Sign in to reply to this message.
On 2012/06/09 12:07:05, kcc1 wrote: > http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... > File sanitizer_common/sanitizer_symbolizer.cc (right): > > http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... > sanitizer_common/sanitizer_symbolizer.cc:124: static Symbolizer *symbolizer; > I am not asking you to use a global with a CTOR, which we indeed ban. > I ask you to create a linker initialized POD object which will have an Init > method. Done. > On 2012/06/09 10:41:48, samsonov wrote: > > On 2012/06/08 12:28:07, kcc1 wrote: > > > can't it be a global object with Init() member? > > > This will avoid the need to InternalALloc call > > > > sanitizer_common/ is a part of TSan RTL, and global ctors are forbidden there > > (so the code just won't compile). > > http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... > File sanitizer_common/sanitizer_symbolizer.h (right): > > http://codereview.appspot.com/6308048/diff/3002/sanitizer_common/sanitizer_sy... > sanitizer_common/sanitizer_symbolizer.h:56: // This function is (currently) NOT > thread-safe. > So, why don't you write exactly that Done. > > On 2012/06/09 10:41:48, samsonov wrote: > > On 2012/06/08 12:28:07, kcc1 wrote: > > > what does this mean? > > > > Two threads shouldn't call SymbolizeCode(...) simultaneously.
Sign in to reply to this message.
http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc File asan/asan_stack.cc (right): http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc#newcode48 asan/asan_stack.cc:48: AddressInfo addr_frames[5]; 5 does not look like an absolute limit on number of inlined frames. I would increase it to something like 64. http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc#newcode51 asan/asan_stack.cc:51: for (uptr j = 0; j < addr_frames_num; j++) { This way we will end up with empty stacks. If we failed to symbolize we need to print at least pc/module+offset. http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc#newcode62 asan/asan_stack.cc:62: proc_maps.Reset(); Do we need this? How was it working w/o Reset()? http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... File sanitizer_common/sanitizer_symbolizer.cc (right): http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... sanitizer_common/sanitizer_symbolizer.cc:76: bool FillAddressInfo(uptr addr, AddressInfo *info) { bool is a bit confusing here, it looks like success/failure, but you use it number of frames. http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... sanitizer_common/sanitizer_symbolizer.cc:86: // Don't subtract 'start' for the first entry. Don't ask me why. Are you sure that is true for -pie?
Sign in to reply to this message.
http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc File asan/asan_stack.cc (right): http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc#newcode48 asan/asan_stack.cc:48: AddressInfo addr_frames[5]; On 2012/06/11 02:40:16, dvyukov wrote: > 5 does not look like an absolute limit on number of inlined frames. I would > increase it to something like 64. Done. http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc#newcode51 asan/asan_stack.cc:51: for (uptr j = 0; j < addr_frames_num; j++) { On 2012/06/11 02:40:16, dvyukov wrote: > This way we will end up with empty stacks. > If we failed to symbolize we need to print at least pc/module+offset. Done. http://codereview.appspot.com/6308048/diff/10001/asan/asan_stack.cc#newcode62 asan/asan_stack.cc:62: proc_maps.Reset(); On 2012/06/11 02:40:16, dvyukov wrote: > Do we need this? How was it working w/o Reset()? Currently GetObjectNameAndOffset calls "Reset" itself (which is probably the place to do it, it's not a caller's responsibility after all). Removed this line. http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... File sanitizer_common/sanitizer_symbolizer.cc (right): http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... sanitizer_common/sanitizer_symbolizer.cc:76: bool FillAddressInfo(uptr addr, AddressInfo *info) { On 2012/06/11 02:40:16, dvyukov wrote: > bool is a bit confusing here, it looks like success/failure, but you use it > number of frames. Done. http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... sanitizer_common/sanitizer_symbolizer.cc:86: // Don't subtract 'start' for the first entry. Don't ask me why. On 2012/06/11 02:40:16, dvyukov wrote: > Are you sure that is true for -pie? Not sure, but looks like that ("me" is not me here).
Sign in to reply to this message.
http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... File sanitizer_common/sanitizer_symbolizer.cc (right): http://codereview.appspot.com/6308048/diff/10001/sanitizer_common/sanitizer_s... sanitizer_common/sanitizer_symbolizer.cc:86: // Don't subtract 'start' for the first entry. Don't ask me why. On 2012/06/14 08:11:07, samsonov wrote: > On 2012/06/11 02:40:16, dvyukov wrote: > > Are you sure that is true for -pie? > > Not sure, but looks like that ("me" is not me here). May you verify that it works with -pie? I suspect that it will print some meaningless address line 0x7fxxxxxxxxxx. "Don't ask me why" is not very useful comment... http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc File asan/asan_stack.cc (right): http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51 asan/asan_stack.cc:51: if (addr_frames_num == 0) { Can't we just fallback here to that other branch below? I think module name stil can be useful (is it libc or jit or what?).
Sign in to reply to this message.
The module name/offset is correct if I run output tests with CXXFLAGS += -fPIC -pie. http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc File asan/asan_stack.cc (right): http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51 asan/asan_stack.cc:51: if (addr_frames_num == 0) { On 2012/06/14 14:28:00, dvyukov wrote: > Can't we just fallback here to that other branch below? > I think module name stil can be useful (is it libc or jit or what?). Done. Note that we'd probably always use symbolize=1 and erase the part below, as SymbolizeCode obtains module name/offset itself.
Sign in to reply to this message.
On Thu, Jun 14, 2012 at 11:05 AM, <samsonov@google.com> wrote: > The module name/offset is correct if I run output tests with CXXFLAGS += > -fPIC -pie. > <http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> > You need to check "the first" module, the one that comes first in /proc/self/maps. I am pretty skeptical that it works. > http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc<http:/... > File asan/asan_stack.cc (right): > > http://codereview.appspot.com/**6308048/diff/5003/asan/asan_** > stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> > asan/asan_stack.cc:51: if (addr_frames_num == 0) { > On 2012/06/14 14:28:00, dvyukov wrote: > >> Can't we just fallback here to that other branch below? >> I think module name stil can be useful (is it libc or jit or what?). >> > > Done. Note that we'd probably always use symbolize=1 and erase the part > below, as SymbolizeCode obtains module name/offset itself. > > http://codereview.appspot.com/**6308048/<http://codereview.appspot.com/6308048/> >
Sign in to reply to this message.
On Thu, Jun 14, 2012 at 7:14 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Thu, Jun 14, 2012 at 11:05 AM, <samsonov@google.com> wrote: > >> The module name/offset is correct if I run output tests with CXXFLAGS += >> -fPIC -pie. >> <http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> >> > > You need to check "the first" module, the one that comes first in > /proc/self/maps. I am pretty skeptical that it works. > IIRC if we use -pie, the code is mapped to the top of the address space, so I doubt that a program module can be "the first one" in the process map - the shadow will be lower (at least for ASan): $ clang++ -faddress-sanitizer heap-overflow.cc $ ./a.out ==25559== Process memory map follows: 0x000000400000-0x00000041c000 /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out 0x00000061c000-0x00000061d000 /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out 0x00000061d000-0x00000061e000 /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out 0x00000061e000-0x000002665000 0x0ffffffff000-0x120000000000 0x120000000000-0x140000000000 0x140000000000-0x200000000000 0x7f14318c7000-0x7f1431a41000 /lib/libc-2.11.1.so 0x7f1431a41000-0x7f1431c40000 /lib/libc-2.11.1.so ... $ clang++ -faddress-sanitizer -fPIC -pie heap-overflow.cc $ ./a.out ==25611== Process memory map follows: 0x0ffffffff000-0x120000000000 0x120000000000-0x140000000000 0x140000000000-0x200000000000 0x7f19880f1000-0x7f198826b000 /lib/libc-2.11.1.so <...> 0x7f1989265000-0x7f1989282000 /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out 0x7f1989481000-0x7f1989482000 /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > > > >> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc<http:/... >> File asan/asan_stack.cc (right): >> >> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_** >> stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> >> asan/asan_stack.cc:51: if (addr_frames_num == 0) { >> On 2012/06/14 14:28:00, dvyukov wrote: >> >>> Can't we just fallback here to that other branch below? >>> I think module name stil can be useful (is it libc or jit or what?). >>> >> >> Done. Note that we'd probably always use symbolize=1 and erase the part >> below, as SymbolizeCode obtains module name/offset itself. >> >> http://codereview.appspot.com/**6308048/<http://codereview.appspot.com/6308048/> >> > > -- Alexey Samsonov, MSK
Sign in to reply to this message.
On Thu, Jun 14, 2012 at 11:47 AM, Alexey Samsonov <samsonov@google.com>wrote: > The module name/offset is correct if I run output tests with CXXFLAGS += >>> -fPIC -pie. >>> <http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> >>> >> >> You need to check "the first" module, the one that comes first in >> /proc/self/maps. I am pretty skeptical that it works. >> > > IIRC if we use -pie, the code is mapped to the top of the address space, > so I doubt that a program module can be "the first one" in the > process map - the shadow will be lower (at least for ASan): > > $ clang++ -faddress-sanitizer heap-overflow.cc > $ ./a.out > ==25559== Process memory map follows: > 0x000000400000-0x00000041c000 > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > 0x00000061c000-0x00000061d000 > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > 0x00000061d000-0x00000061e000 > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > 0x00000061e000-0x000002665000 > 0x0ffffffff000-0x120000000000 > 0x120000000000-0x140000000000 > 0x140000000000-0x200000000000 > 0x7f14318c7000-0x7f1431a41000 /lib/libc-2.11.1.so > 0x7f1431a41000-0x7f1431c40000 /lib/libc-2.11.1.so > ... > $ clang++ -faddress-sanitizer -fPIC -pie heap-overflow.cc > $ ./a.out > ==25611== Process memory map follows: > 0x0ffffffff000-0x120000000000 > 0x120000000000-0x140000000000 > 0x140000000000-0x200000000000 > 0x7f19880f1000-0x7f198826b000 /lib/libc-2.11.1.so > <...> > 0x7f1989265000-0x7f1989282000 > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > 0x7f1989481000-0x7f1989482000 > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > > You need to check "the first" module, the one that comes first in /proc/self/maps. I am pretty skeptical that it works. > >> >> >> >>> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc<http:/... >>> File asan/asan_stack.cc (right): >>> >>> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_** >>> stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> >>> asan/asan_stack.cc:51: if (addr_frames_num == 0) { >>> On 2012/06/14 14:28:00, dvyukov wrote: >>> >>>> Can't we just fallback here to that other branch below? >>>> I think module name stil can be useful (is it libc or jit or what?). >>>> >>> >>> Done. Note that we'd probably always use symbolize=1 and erase the part >>> below, as SymbolizeCode obtains module name/offset itself. >>> >>> http://codereview.appspot.com/**6308048/<http://codereview.appspot.com/6308048/> >>> >> >> > > > -- > Alexey Samsonov, MSK > >
Sign in to reply to this message.
On Thu, Jun 14, 2012 at 9:22 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Thu, Jun 14, 2012 at 11:47 AM, Alexey Samsonov <samsonov@google.com>wrote: > >> The module name/offset is correct if I run output tests with CXXFLAGS += >>>> -fPIC -pie. >>>> <http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> >>>> >>> >>> You need to check "the first" module, the one that comes first in >>> /proc/self/maps. I am pretty skeptical that it works. >>> >> >> IIRC if we use -pie, the code is mapped to the top of the address space, >> so I doubt that a program module can be "the first one" in the >> process map - the shadow will be lower (at least for ASan): >> >> $ clang++ -faddress-sanitizer heap-overflow.cc >> $ ./a.out >> ==25559== Process memory map follows: >> 0x000000400000-0x00000041c000 >> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >> 0x00000061c000-0x00000061d000 >> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >> 0x00000061d000-0x00000061e000 >> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >> 0x00000061e000-0x000002665000 >> 0x0ffffffff000-0x120000000000 >> 0x120000000000-0x140000000000 >> 0x140000000000-0x200000000000 >> 0x7f14318c7000-0x7f1431a41000 /lib/libc-2.11.1.so >> 0x7f1431a41000-0x7f1431c40000 /lib/libc-2.11.1.so >> ... >> $ clang++ -faddress-sanitizer -fPIC -pie heap-overflow.cc >> $ ./a.out >> ==25611== Process memory map follows: >> 0x0ffffffff000-0x120000000000 >> 0x120000000000-0x140000000000 >> 0x140000000000-0x200000000000 >> 0x7f19880f1000-0x7f198826b000 /lib/libc-2.11.1.so >> <...> >> 0x7f1989265000-0x7f1989282000 >> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >> 0x7f1989481000-0x7f1989482000 >> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >> >> > > You need to check "the first" module, the one that comes first in > /proc/self/maps. I am pretty skeptical that it works. > What do you mean? It works for the first module if build binary w/o -pie. If we build binary with -pie, all the modules will be mapped too high, so none of them can be first in /proc/self/maps. > > >> >>> >>> >>> >>>> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc<http:/... >>>> File asan/asan_stack.cc (right): >>>> >>>> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_** >>>> stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> >>>> asan/asan_stack.cc:51: if (addr_frames_num == 0) { >>>> On 2012/06/14 14:28:00, dvyukov wrote: >>>> >>>>> Can't we just fallback here to that other branch below? >>>>> I think module name stil can be useful (is it libc or jit or what?). >>>>> >>>> >>>> Done. Note that we'd probably always use symbolize=1 and erase the part >>>> below, as SymbolizeCode obtains module name/offset itself. >>>> >>>> http://codereview.appspot.com/**6308048/<http://codereview.appspot.com/6308048/> >>>> >>> >>> >> >> >> -- >> Alexey Samsonov, MSK >> >> > -- Alexey Samsonov, MSK
Sign in to reply to this message.
On Fri, Jun 15, 2012 at 5:31 AM, Alexey Samsonov <samsonov@google.com>wrote: > The module name/offset is correct if I run output tests with CXXFLAGS += >>>>> -fPIC -pie. >>>>> <http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> >>>>> >>>> >>>> You need to check "the first" module, the one that comes first in >>>> /proc/self/maps. I am pretty skeptical that it works. >>>> >>> >>> IIRC if we use -pie, the code is mapped to the top of the address space, >>> so I doubt that a program module can be "the first one" in the >>> process map - the shadow will be lower (at least for ASan): >>> >>> $ clang++ -faddress-sanitizer heap-overflow.cc >>> $ ./a.out >>> ==25559== Process memory map follows: >>> 0x000000400000-0x00000041c000 >>> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >>> 0x00000061c000-0x00000061d000 >>> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >>> 0x00000061d000-0x00000061e000 >>> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >>> 0x00000061e000-0x000002665000 >>> 0x0ffffffff000-0x120000000000 >>> 0x120000000000-0x140000000000 >>> 0x140000000000-0x200000000000 >>> 0x7f14318c7000-0x7f1431a41000 /lib/libc-2.11.1.so >>> 0x7f1431a41000-0x7f1431c40000 /lib/libc-2.11.1.so >>> ... >>> $ clang++ -faddress-sanitizer -fPIC -pie heap-overflow.cc >>> $ ./a.out >>> ==25611== Process memory map follows: >>> 0x0ffffffff000-0x120000000000 >>> 0x120000000000-0x140000000000 >>> 0x140000000000-0x200000000000 >>> 0x7f19880f1000-0x7f198826b000 /lib/libc-2.11.1.so >>> <...> >>> 0x7f1989265000-0x7f1989282000 >>> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >>> 0x7f1989481000-0x7f1989482000 >>> /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out >>> >>> >> >> You need to check "the first" module, the one that comes first in >> /proc/self/maps. I am pretty skeptical that it works. >> > > What do you mean? It works for the first module if build binary w/o -pie. > If we build binary with -pie, all the modules will be mapped > too high, so none of them can be first in /proc/self/maps. > Please add that to the comment. >>>> >>>>> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc<http:/... >>>>> File asan/asan_stack.cc (right): >>>>> >>>>> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_** >>>>> stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> >>>>> asan/asan_stack.cc:51: if (addr_frames_num == 0) { >>>>> On 2012/06/14 14:28:00, dvyukov wrote: >>>>> >>>>>> Can't we just fallback here to that other branch below? >>>>>> I think module name stil can be useful (is it libc or jit or what?). >>>>>> >>>>> >>>>> Done. Note that we'd probably always use symbolize=1 and erase the part >>>>> below, as SymbolizeCode obtains module name/offset itself. >>>>> >>>>> http://codereview.appspot.com/**6308048/<http://codereview.appspot.com/6308048/> >>>>> >>>> >>>> >>> >>> >>> -- >>> Alexey Samsonov, MSK >>> >>> >> > > > -- > Alexey Samsonov, MSK > >
Sign in to reply to this message.
On 2012/06/15 12:55:31, dvyukov wrote: > On Fri, Jun 15, 2012 at 5:31 AM, Alexey Samsonov <samsonov@google.com>wrote: > > > The module name/offset is correct if I run output tests with CXXFLAGS += > >>>>> -fPIC -pie. > >>>>> <http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> > >>>>> > >>>> > >>>> You need to check "the first" module, the one that comes first in > >>>> /proc/self/maps. I am pretty skeptical that it works. > >>>> > >>> > >>> IIRC if we use -pie, the code is mapped to the top of the address space, > >>> so I doubt that a program module can be "the first one" in the > >>> process map - the shadow will be lower (at least for ASan): > >>> > >>> $ clang++ -faddress-sanitizer heap-overflow.cc > >>> $ ./a.out > >>> ==25559== Process memory map follows: > >>> 0x000000400000-0x00000041c000 > >>> > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > >>> 0x00000061c000-0x00000061d000 > >>> > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > >>> 0x00000061d000-0x00000061e000 > >>> > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > >>> 0x00000061e000-0x000002665000 > >>> 0x0ffffffff000-0x120000000000 > >>> 0x120000000000-0x140000000000 > >>> 0x140000000000-0x200000000000 > >>> 0x7f14318c7000-0x7f1431a41000 /lib/libc-2.11.1.so > >>> 0x7f1431a41000-0x7f1431c40000 /lib/libc-2.11.1.so > >>> ... > >>> $ clang++ -faddress-sanitizer -fPIC -pie heap-overflow.cc > >>> $ ./a.out > >>> ==25611== Process memory map follows: > >>> 0x0ffffffff000-0x120000000000 > >>> 0x120000000000-0x140000000000 > >>> 0x140000000000-0x200000000000 > >>> 0x7f19880f1000-0x7f198826b000 /lib/libc-2.11.1.so > >>> <...> > >>> 0x7f1989265000-0x7f1989282000 > >>> > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > >>> 0x7f1989481000-0x7f1989482000 > >>> > /home/samsonov/llvm-project/llvm2/projects/compiler-rt/lib/asan/output_tests/a.out > >>> > >>> > >> > >> You need to check "the first" module, the one that comes first in > >> /proc/self/maps. I am pretty skeptical that it works. > >> > > > > What do you mean? It works for the first module if build binary w/o -pie. > > If we build binary with -pie, all the modules will be mapped > > too high, so none of them can be first in /proc/self/maps. > > > > Please add that to the comment. Done. > > > > >>>> > >>>>> > http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc%3Chttp...> > >>>>> File asan/asan_stack.cc (right): > >>>>> > >>>>> http://codereview.appspot.com/**6308048/diff/5003/asan/asan_** > >>>>> > stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> > >>>>> asan/asan_stack.cc:51: if (addr_frames_num == 0) { > >>>>> On 2012/06/14 14:28:00, dvyukov wrote: > >>>>> > >>>>>> Can't we just fallback here to that other branch below? > >>>>>> I think module name stil can be useful (is it libc or jit or what?). > >>>>>> > >>>>> > >>>>> Done. Note that we'd probably always use symbolize=1 and erase the part > >>>>> below, as SymbolizeCode obtains module name/offset itself. > >>>>> > >>>>> > http://codereview.appspot.com/**6308048/%3Chttp://codereview.appspot.com/6308...> > >>>>> > >>>> > >>>> > >>> > >>> > >>> -- > >>> Alexey Samsonov, MSK > >>> > >>> > >> > > > > > > -- > > Alexey Samsonov, MSK > > > >
Sign in to reply to this message.
LGTM On Fri, Jun 15, 2012 at 9:25 AM, <samsonov@google.com> wrote: > On 2012/06/15 12:55:31, dvyukov wrote: > >> On Fri, Jun 15, 2012 at 5:31 AM, Alexey Samsonov >> > <samsonov@google.com>wrote: > > > The module name/offset is correct if I run output tests with >> > CXXFLAGS += > >> >>>>> -fPIC -pie. >> >>>>> >> > <http://codereview.appspot.**com/6308048/diff/5003/asan/**asan_stack.cc<http:/... > > > > >>>>> >> >>>> >> >>>> You need to check "the first" module, the one that comes first in >> >>>> /proc/self/maps. I am pretty skeptical that it works. >> >>>> >> >>> >> >>> IIRC if we use -pie, the code is mapped to the top of the address >> > space, > >> >>> so I doubt that a program module can be "the first one" in the >> >>> process map - the shadow will be lower (at least for ASan): >> >>> >> >>> $ clang++ -faddress-sanitizer heap-overflow.cc >> >>> $ ./a.out >> >>> ==25559== Process memory map follows: >> >>> 0x000000400000-0x00000041c000 >> >>> >> > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > lib/asan/output_tests/a.out > >> >>> 0x00000061c000-0x00000061d000 >> >>> >> > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > lib/asan/output_tests/a.out > >> >>> 0x00000061d000-0x00000061e000 >> >>> >> > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > lib/asan/output_tests/a.out > >> >>> 0x00000061e000-0x000002665000 >> >>> 0x0ffffffff000-0x120000000000 >> >>> 0x120000000000-0x140000000000 >> >>> 0x140000000000-0x200000000000 >> >>> 0x7f14318c7000-0x7f1431a41000 /lib/libc-2.11.1.so >> >>> 0x7f1431a41000-0x7f1431c40000 /lib/libc-2.11.1.so >> >>> ... >> >>> $ clang++ -faddress-sanitizer -fPIC -pie heap-overflow.cc >> >>> $ ./a.out >> >>> ==25611== Process memory map follows: >> >>> 0x0ffffffff000-0x120000000000 >> >>> 0x120000000000-0x140000000000 >> >>> 0x140000000000-0x200000000000 >> >>> 0x7f19880f1000-0x7f198826b000 /lib/libc-2.11.1.so >> >>> <...> >> >>> 0x7f1989265000-0x7f1989282000 >> >>> >> > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > lib/asan/output_tests/a.out > >> >>> 0x7f1989481000-0x7f1989482000 >> >>> >> > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > lib/asan/output_tests/a.out > >> >>> >> >>> >> >> >> >> You need to check "the first" module, the one that comes first in >> >> /proc/self/maps. I am pretty skeptical that it works. >> >> >> > >> > What do you mean? It works for the first module if build binary w/o >> > -pie. > >> > If we build binary with -pie, all the modules will be mapped >> > too high, so none of them can be first in /proc/self/maps. >> > >> > > Please add that to the comment. >> > > Done. > > > > > > >>>> >> >>>>> >> > > http://codereview.appspot.com/****6308048/diff/5003/asan/asan_** > **stack.cc%3Chttp://**codereview.appspot.com/** > 6308048/diff/5003/asan/asan_**stack.cc<http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc%3Chttp://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> > > > >> >>>>> File asan/asan_stack.cc (right): >> >>>>> >> >>>>> http://codereview.appspot.com/****6308048/diff/5003/asan/asan_****<http://cod... >> >>>>> >> > > stack.cc#newcode51<http://**codereview.appspot.com/** > 6308048/diff/5003/asan/asan_**stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> > > > > >>>>> asan/asan_stack.cc:51: if (addr_frames_num == 0) { >> >>>>> On 2012/06/14 14:28:00, dvyukov wrote: >> >>>>> >> >>>>>> Can't we just fallback here to that other branch below? >> >>>>>> I think module name stil can be useful (is it libc or jit or >> > what?). > >> >>>>>> >> >>>>> >> >>>>> Done. Note that we'd probably always use symbolize=1 and erase >> > the part > >> >>>>> below, as SymbolizeCode obtains module name/offset itself. >> >>>>> >> >>>>> >> > > http://codereview.appspot.com/****6308048/%3Chttp://** > codereview.appspot.com/**6308048/<http://codereview.appspot.com/**6308048/%3Chttp://codereview.appspot.com/6308048/> > > > > >>>>> >> >>>> >> >>>> >> >>> >> >>> >> >>> -- >> >>> Alexey Samsonov, MSK >> >>> >> >>> >> >> >> > >> > >> > -- >> > Alexey Samsonov, MSK >> > >> > >> > > > > http://codereview.appspot.com/**6308048/<http://codereview.appspot.com/6308048/> >
Sign in to reply to this message.
On 2012/06/15 13:34:17, dvyukov wrote: > LGTM r158522, thanks! > > On Fri, Jun 15, 2012 at 9:25 AM, <mailto:samsonov@google.com> wrote: > > > On 2012/06/15 12:55:31, dvyukov wrote: > > > >> On Fri, Jun 15, 2012 at 5:31 AM, Alexey Samsonov > >> > > <samsonov@google.com>wrote: > > > > > The module name/offset is correct if I run output tests with > >> > > CXXFLAGS += > > > >> >>>>> -fPIC -pie. > >> >>>>> > >> > > > <http://codereview.appspot.**com/6308048/diff/5003/asan/**asan_stack.cc%3Chttp...> > > > > > > > >>>>> > >> >>>> > >> >>>> You need to check "the first" module, the one that comes first in > >> >>>> /proc/self/maps. I am pretty skeptical that it works. > >> >>>> > >> >>> > >> >>> IIRC if we use -pie, the code is mapped to the top of the address > >> > > space, > > > >> >>> so I doubt that a program module can be "the first one" in the > >> >>> process map - the shadow will be lower (at least for ASan): > >> >>> > >> >>> $ clang++ -faddress-sanitizer heap-overflow.cc > >> >>> $ ./a.out > >> >>> ==25559== Process memory map follows: > >> >>> 0x000000400000-0x00000041c000 > >> >>> > >> > > > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > > lib/asan/output_tests/a.out > > > >> >>> 0x00000061c000-0x00000061d000 > >> >>> > >> > > > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > > lib/asan/output_tests/a.out > > > >> >>> 0x00000061d000-0x00000061e000 > >> >>> > >> > > > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > > lib/asan/output_tests/a.out > > > >> >>> 0x00000061e000-0x000002665000 > >> >>> 0x0ffffffff000-0x120000000000 > >> >>> 0x120000000000-0x140000000000 > >> >>> 0x140000000000-0x200000000000 > >> >>> 0x7f14318c7000-0x7f1431a41000 /lib/libc-2.11.1.so > >> >>> 0x7f1431a41000-0x7f1431c40000 /lib/libc-2.11.1.so > >> >>> ... > >> >>> $ clang++ -faddress-sanitizer -fPIC -pie heap-overflow.cc > >> >>> $ ./a.out > >> >>> ==25611== Process memory map follows: > >> >>> 0x0ffffffff000-0x120000000000 > >> >>> 0x120000000000-0x140000000000 > >> >>> 0x140000000000-0x200000000000 > >> >>> 0x7f19880f1000-0x7f198826b000 /lib/libc-2.11.1.so > >> >>> <...> > >> >>> 0x7f1989265000-0x7f1989282000 > >> >>> > >> > > > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > > lib/asan/output_tests/a.out > > > >> >>> 0x7f1989481000-0x7f1989482000 > >> >>> > >> > > > > /home/samsonov/llvm-project/**llvm2/projects/compiler-rt/** > > lib/asan/output_tests/a.out > > > >> >>> > >> >>> > >> >> > >> >> You need to check "the first" module, the one that comes first in > >> >> /proc/self/maps. I am pretty skeptical that it works. > >> >> > >> > > >> > What do you mean? It works for the first module if build binary w/o > >> > > -pie. > > > >> > If we build binary with -pie, all the modules will be mapped > >> > too high, so none of them can be first in /proc/self/maps. > >> > > >> > > > > Please add that to the comment. > >> > > > > Done. > > > > > > > > > > > > >>>> > >> >>>>> > >> > > > > http://codereview.appspot.com/****6308048/diff/5003/asan/asan_** > > **stack.cc%3Chttp://**codereview.appspot.com/** > > > 6308048/diff/5003/asan/asan_**stack.cc<http://codereview.appspot.com/**6308048/diff/5003/asan/asan_**stack.cc%3Chttp://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc> > > > > > > >> >>>>> File asan/asan_stack.cc (right): > >> >>>>> > >> >>>>> > http://codereview.appspot.com/****6308048/diff/5003/asan/asan_****%3Chttp://c...> > >> >>>>> > >> > > > > stack.cc#newcode51<http://**codereview.appspot.com/** > > > 6308048/diff/5003/asan/asan_**stack.cc#newcode51<http://codereview.appspot.com/6308048/diff/5003/asan/asan_stack.cc#newcode51> > > > > > > > >>>>> asan/asan_stack.cc:51: if (addr_frames_num == 0) { > >> >>>>> On 2012/06/14 14:28:00, dvyukov wrote: > >> >>>>> > >> >>>>>> Can't we just fallback here to that other branch below? > >> >>>>>> I think module name stil can be useful (is it libc or jit or > >> > > what?). > > > >> >>>>>> > >> >>>>> > >> >>>>> Done. Note that we'd probably always use symbolize=1 and erase > >> > > the part > > > >> >>>>> below, as SymbolizeCode obtains module name/offset itself. > >> >>>>> > >> >>>>> > >> > > > > http://codereview.appspot.com/****6308048/%253Chttp://** > > > codereview.appspot.com/**6308048/<http://codereview.appspot.com/**6308048/%3Chttp://codereview.appspot.com/6308048/> > > > > > > > >>>>> > >> >>>> > >> >>>> > >> >>> > >> >>> > >> >>> -- > >> >>> Alexey Samsonov, MSK > >> >>> > >> >>> > >> >> > >> > > >> > > >> > -- > >> > Alexey Samsonov, MSK > >> > > >> > > >> > > > > > > > > > http://codereview.appspot.com/**6308048/%3Chttp://codereview.appspot.com/6308...> > >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
