[PATCH] Interceptors etc for standalone LSan.
Sergey Matveev
earthdok at google.com
Tue Apr 23 07:51:24 PDT 2013
================
Comment at: lib/lsan/lsan_allocator.h:24
@@ +23,3 @@
+
+struct ChunkMetadata {
+ uptr requested_size;
----------------
Dmitry Vyukov wrote:
> In sanitizer_common, asan and tsan we use "block" term for this. Please use "block" instead of "chunk" here and elsewhere.
>
Keeping the current naming as per the offline discussion.
================
Comment at: lib/lsan/lsan_allocator.h:33
@@ +32,3 @@
+
+void *Allocate(StackTrace const &stack, uptr size, uptr alignment,
+ bool cleared = false);
----------------
Dmitry Vyukov wrote:
> In the codebase we use "const T &" instead of "T const&"
fixed
================
Comment at: lib/lsan/lsan_allocator.h:34
@@ +33,3 @@
+void *Allocate(StackTrace const &stack, uptr size, uptr alignment,
+ bool cleared = false);
+void Deallocate(void *p);
----------------
Dmitry Vyukov wrote:
> Sergey Matveev wrote:
> > Dmitry Vyukov wrote:
> > > Alexander Potapenko wrote:
> > > > I think it's better to avoid the default parameter here.
> > > Default values are discouraged by the code style. I think you will call this in very few places anyway, so passing it explicitly is fine.
> > Why is it ok to have it in Allocate() in sanitizer_allocator.h but not here?
> It is not OK there as well.
fixed
================
Comment at: lib/lsan/lsan_allocator.cc:17
@@ +16,3 @@
+
+#include <pthread.h>
+
----------------
Dmitry Vyukov wrote:
> Do you need this?
removed
================
Comment at: lib/lsan/lsan_allocator.cc:46
@@ +45,3 @@
+ Init();
+ inited = true;
+ allocator.Init();
----------------
Dmitry Vyukov wrote:
> move this one line up
fixed
================
Comment at: lib/lsan/lsan_allocator.cc:58
@@ +57,3 @@
+
+void RegisterAllocation(StackTrace const &stack, void *p, uptr size) {
+ if (!p) return;
----------------
Dmitry Vyukov wrote:
> s/void/static void/
fixed
================
Comment at: lib/lsan/lsan_allocator.cc:81
@@ +80,3 @@
+ // ensures that leak checking is not taking place now.
+ return NULL;
+}
----------------
Dmitry Vyukov wrote:
> s/NULL/0/
fixed
================
Comment at: lib/lsan/lsan_interceptors.cc:16
@@ +15,3 @@
+#include <stddef.h>
+#include <stdlib.h> // setenv
+#include <string.h> // memset
----------------
Dmitry Vyukov wrote:
> Is lint happy with this? it should say "2 spaces between code and comment".
> Such comments are usually useless and outdated.
>
fixed
================
Comment at: lib/lsan/lsan_interceptors.cc:31
@@ +30,3 @@
+
+extern "C" int pthread_attr_init(void *attr);
+extern "C" int pthread_attr_destroy(void *attr);
----------------
Alexander Potapenko wrote:
> extern "C" {
> ...
> }
fixed
================
Comment at: lib/lsan/lsan_interceptors.cc:46
@@ +45,3 @@
+ Init();
+ GET_STACK_TRACE(flags()->stack_trace_size, flags()->fast_unwind);
+ void *p = Allocate(stack, size, 8);
----------------
Alexander Potapenko wrote:
> Looks like you're always calling GET_STACK_TRACE(flags()->stack_trace_size, flags()->fast_unwind). Can it be just GET_STACK_TRACE then?
ok
================
Comment at: lib/lsan/lsan_interceptors.cc:47
@@ +46,3 @@
+ GET_STACK_TRACE(flags()->stack_trace_size, flags()->fast_unwind);
+ void *p = Allocate(stack, size, 8);
+ return p;
----------------
Dmitry Vyukov wrote:
> const int kAlignment = 8;
ok
================
Comment at: lib/lsan/lsan_interceptors.cc:59
@@ +58,3 @@
+ GET_STACK_TRACE(flags()->stack_trace_size, flags()->fast_unwind);
+ size *= nmemb;
+ void *p = Allocate(stack, size, 8, false);
----------------
Alexander Potapenko wrote:
> Dmitry Vyukov wrote:
> > check for integer overflow
> Multiplication overflow here leads to a security vulnerability. Please use CallocShouldReturnNullDueToOverflow() from sanitizer_common.h
ok
================
Comment at: lib/lsan/lsan_interceptors.cc:83
@@ +82,3 @@
+ *memptr = Allocate(stack, size, alignment);
+ return 0;
+}
----------------
Dmitry Vyukov wrote:
> ENOMEM if the pointer is 0
I fixed it, but be aware that no other tool does this.
================
Comment at: lib/lsan/lsan_interceptors.cc:96
@@ +95,3 @@
+INTERCEPTOR(void, malloc_usable_size, void) {
+}
+
----------------
Dmitry Vyukov wrote:
> Is it not implemented yet or intentionally empty?
> If the former, then add a comment saying "not implemented".
ok
================
Comment at: lib/lsan/lsan_interceptors.cc:107
@@ +106,3 @@
+void *operator new(size_t size, std::nothrow_t const&) ALIAS("malloc")
+ SANITIZER_INTERFACE_ATTRIBUTE;
+void *operator new[](size_t size, std::nothrow_t const&) ALIAS("malloc")
----------------
Alexander Potapenko wrote:
> Four spaces here and below, unless you're formatting everything with clang-format.
ok
================
Comment at: lib/lsan/lsan_thread.cc:25
@@ +24,3 @@
+
+extern "C" int arch_prctl(int code, __sanitizer::uptr *addr);
+
----------------
Alexander Potapenko wrote:
> Isn't arch_prctl declared in asm/prctl.h?
no
================
Comment at: lib/lsan/lsan_thread.cc:31
@@ +30,3 @@
+
+ThreadRegistry *thread_registry;
+BlockingMutex thread_create_mutex(LINKER_INITIALIZED);
----------------
Dmitry Vyukov wrote:
> static
ok
================
Comment at: lib/lsan/lsan_thread.cc:47
@@ +46,3 @@
+ thread_registry = new(thread_registry_placeholder)
+ ThreadRegistry(CreateThreadContext, 1 << 13, 64);
+}
----------------
Alexander Potapenko wrote:
> What are 1<<13 and 64?
clarified
================
Comment at: lib/lsan/lsan_thread.cc:60
@@ +59,3 @@
+ : ThreadContextBase(tid),
+ stack_begin_(0), stack_end_(0),
+ cache_begin_(0), cache_end_(0),
----------------
Dmitry Vyukov wrote:
> one statement on line plz
ok
================
Comment at: lib/lsan/lsan_thread.cc:86
@@ +85,3 @@
+ return thread_registry->CreateThread(user_id, detached,
+ parent_tid, NULL);
+}
----------------
Dmitry Vyukov wrote:
> Alexander Potapenko wrote:
> > Please fix the indentation.
> s/NULL/0/
fixed
================
Comment at: lib/lsan/lsan_thread.cc:92
@@ +91,3 @@
+#ifdef __x86_64__
+#define TCB_SIZE 2304
+#endif
----------------
Dmitry Vyukov wrote:
> use const int
ok
================
Comment at: lib/lsan/lsan_thread.cc:125
@@ +124,3 @@
+ uptr tls_size = 0;
+ GetThreadStackAndTls(tid == 0, &stack_addr, &stack_size,
+ &tls_addr, &tls_size);
----------------
Dmitry Vyukov wrote:
> use &args.stack_begin here and remove the standalone vars
ok
================
Comment at: lib/lsan/lsan_thread.cc:141
@@ +140,3 @@
+ if (!thread_registry) return NULL;
+ if (GetCurrentThread() == (u32)-1)
+ return NULL;
----------------
Dmitry Vyukov wrote:
> s/(u32)-1/kInvalidTid/
ok
================
Comment at: lib/lsan/lsan_thread.cc:145
@@ +144,3 @@
+ return (ThreadContext *)thread_registry->
+ GetThreadLocked(GetCurrentThread());
+}
----------------
Alexander Potapenko wrote:
> Unless the two spaces have been suggested by clang-format, please use four-space indentation here.
> I also think it's more readable to avoid line wrapping here.
fixed
http://llvm-reviews.chandlerc.com/D702
More information about the llvm-commits
mailing list