Could you also run `clang-format` on the code? Preferably separate from other
changes to the diff history is easier to read.
================
Comment at: cmake/config-ix.cmake:259
@@ -257,2 +258,3 @@
mipsel mips64 mips64el powerpc64 powerpc64le)
+filter_available_targets(SAFESTACK_SUPPORTED_ARCH x86_64 i386 i686)
----------------
I haven't been through the LLVM-side code yet, but what prevents other
architectures from being supported? IIUC nothing is x86 specific since it's in
``lib/Transforms``, right?
================
Comment at: cmake/config-ix.cmake:340
@@ +339,3 @@
+if (SAFESTACK_SUPPORTED_ARCH AND
+ OS_NAME MATCHES "Darwin|Linux|FreeBSD")
+ set(COMPILER_RT_HAS_SAFESTACK TRUE)
----------------
What's missing for Windows support? Thread and TLS support? It's the most
significant platform for Chrome (well, and Android), and we're looking forward
to LLVM support on Windows. Not having SafeStack would be sad. This can be a
different patch, but I think it's important.
================
Comment at: lib/safestack/safestack.cc:44
@@ +43,3 @@
+ __attribute__((visibility ("default")))
+ __thread void *__safestack_unsafe_stack_ptr = 0;
+}
----------------
`nullptr` here and other pointers below.
================
Comment at: lib/safestack/safestack.cc:54
@@ +53,3 @@
+static inline void *unsafe_stack_alloc(size_t size, size_t guard) {
+ void *addr = MmapOrDie(size + guard, "unsafe_stack_alloc");
+ MprotectNoAccess((uptr)addr, (uptr)guard);
----------------
It's probably not a huge issue for the unsafe stack, but I'd rather check that
`size + guard` doesn't overflow `uptr` before calling `MmapOrDie`. The same
applies in other parts of the code below.
================
Comment at: lib/safestack/safestack.cc:71
@@ +70,3 @@
+ if (unsafe_stack_start) {
+ (void)UnmapOrDie(
+ (char*) unsafe_stack_start - unsafe_stack_guard,
----------------
Drop the `(void)` case.
================
Comment at: lib/safestack/safestack.cc:106
@@ +105,3 @@
+ // FIXME: we can do this only any other specific key is set by
+ // intersepting the pthread_setspecific function itself
+ pthread_setspecific(thread_cleanup_key, (void*) 1);
----------------
s/intersepting/intercepting/
================
Comment at: lib/safestack/safestack.cc:109
@@ +108,3 @@
+
+ // Start the original thread rutine
+ return start_routine(start_routine_arg);
----------------
s/rutine/routine/
Though I'm not sure the comment is really helping me understand much here, I'd
just drop it.
================
Comment at: lib/safestack/safestack.cc:174
@@ +173,3 @@
+ if (initialized)
+ return;
+
----------------
Is this initialization check useful if `__attribute__((constructor(0)))` was
used? Can this be done concurrently, or is it just called multiple times from
the same thread of execution? If concurrent then initialization is racy.
================
Comment at: lib/safestack/safestack.cc:225
@@ +224,3 @@
+void *__get_safe_stack_ptr() {
+ return (char*) __builtin_frame_address(0) + 2*sizeof(void*);
+}
----------------
Please add a comment on why `2*sizeof(void*)` is correct.
================
Comment at: test/safestack/CMakeLists.txt:9
@@ +8,3 @@
+ list(APPEND SAFESTACK_TEST_DEPS
+ LLVMgold
+ )
----------------
Does this force-build gold for this test? Or does it refuse to build the test
if gold wasn't built? Also, comment on why PIC and+binutils_incdir require gold
for testing.
================
Comment at: test/safestack/CMakeLists.txt:14
@@ +13,3 @@
+ list(APPEND SAFESTACK_TEST_DEPS
+ LTO
+ )
----------------
Why does Apple require LTO for testing?
================
Comment at: test/safestack/check-buffer-copy.c:9
@@ +8,3 @@
+ int i;
+ char buffer[128];
+
----------------
Could you do the same test with a VLA?
================
Comment at: test/safestack/check-buffer-copy.c:14
@@ +13,3 @@
+ buffer[i] = argv[0][i];
+ buffer[i] = '\0';
+
----------------
Writing and then immediately reading back is pretty brittle if the optimizer
decides to kick in and prove the code trivially dead. Could you add some
optimizer smarts defeat mechanism here? I'm not sure if compiler-rt has a
preferred method of doing this (asm volatile, barrier, volatile, separate
noinline call, ...).
================
Comment at: test/safestack/check-overflow.c:10
@@ +9,3 @@
+
+void fct(int *buffer)
+{
----------------
noinline would be appropriate, though IPO could still kick in. Maybe slap a
volatile on there too?
================
Comment at: test/safestack/check-overflow.c:13
@@ +12,3 @@
+ buffer[-1] = 36;
+ buffer[6] = 36;
+}
----------------
I'd rather memset a bigger range which includes the buffer itself than just
changing two values.
================
Comment at: test/safestack/check-overflow.c:22
@@ +21,3 @@
+ fct(buffer);
+ return value1 != 42 || value2 != 42;
+}
----------------
The optimizer knows that `value1` and `value2` don't escape `main`. This check
is trivially true.
================
Comment at: test/safestack/check-pthread.c:22
@@ +21,3 @@
+ char buffer[8096]; // two pages
+ memset(buffer, val, sizeof (buffer));
+
----------------
The buffer allocation and memset are dead code and can be eliminated.
http://reviews.llvm.org/D6096
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
