Improve and fix the stack-protector tests.
Bug: http://b/26888853
(cherry picked from commit fc69a8ad5f0d9b63de48e3858fb4811ede7ac64e)
Change-Id: Ibc431076000b9a8db46f68f858480045b03b6e79
diff --git a/libc/bionic/__libc_init_main_thread.cpp b/libc/bionic/__libc_init_main_thread.cpp
index e1445cb..a29d8a7 100644
--- a/libc/bionic/__libc_init_main_thread.cpp
+++ b/libc/bionic/__libc_init_main_thread.cpp
@@ -74,6 +74,10 @@
main_thread.attr.stack_size = 0; // User code should never see this; we'll compute it when asked.
// TODO: the main thread's sched_policy and sched_priority need to be queried.
+ // The TLS stack guard is set from the global, so ensure that we've initialized the global
+ // before we initialize the TLS.
+ __libc_init_global_stack_chk_guard(args);
+
__init_thread(&main_thread);
__init_tls(&main_thread);
diff --git a/libc/bionic/libc_init_common.cpp b/libc/bionic/libc_init_common.cpp
index ebd1595..c2a5fed 100644
--- a/libc/bionic/libc_init_common.cpp
+++ b/libc/bionic/libc_init_common.cpp
@@ -64,10 +64,17 @@
// Declared in "private/bionic_ssp.h".
uintptr_t __stack_chk_guard = 0;
+void __libc_init_global_stack_chk_guard(KernelArgumentBlock& args) {
+ // AT_RANDOM is a pointer to 16 bytes of randomness on the stack.
+ // Take the first 4/8 for the -fstack-protector implementation.
+ __stack_chk_guard = *reinterpret_cast<uintptr_t*>(args.getauxval(AT_RANDOM));
+}
+
void __libc_init_globals(KernelArgumentBlock& args) {
// Initialize libc globals that are needed in both the linker and in libc.
// In dynamic binaries, this is run at least twice for different copies of the
// globals, once for the linker's copy and once for the one in libc.so.
+ __libc_init_global_stack_chk_guard(args);
__libc_auxv = args.auxv;
__libc_globals.initialize();
__libc_globals.mutate([&args](libc_globals* globals) {
@@ -83,9 +90,6 @@
__progname = args.argv[0] ? args.argv[0] : "<unknown>";
__abort_message_ptr = args.abort_message_ptr;
- // AT_RANDOM is a pointer to 16 bytes of randomness on the stack.
- __stack_chk_guard = *reinterpret_cast<uintptr_t*>(getauxval(AT_RANDOM));
-
// Get the main thread from TLS and add it to the thread list.
pthread_internal_t* main_thread = __get_thread();
__pthread_internal_add(main_thread);
diff --git a/libc/private/bionic_globals.h b/libc/private/bionic_globals.h
index c802e3a..bcb05a0 100644
--- a/libc/private/bionic_globals.h
+++ b/libc/private/bionic_globals.h
@@ -25,10 +25,12 @@
* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
+
#ifndef _PRIVATE_BIONIC_GLOBALS_H
#define _PRIVATE_BIONIC_GLOBALS_H
#include <sys/cdefs.h>
+
#include "private/bionic_malloc_dispatch.h"
#include "private/bionic_vdso.h"
#include "private/WriteProtected.h"
@@ -42,9 +44,9 @@
__LIBC_HIDDEN__ extern WriteProtected<libc_globals> __libc_globals;
class KernelArgumentBlock;
-__LIBC_HIDDEN__ void __libc_init_vdso(libc_globals* globals,
- KernelArgumentBlock& args);
-__LIBC_HIDDEN__ void __libc_init_setjmp_cookie(libc_globals* globals,
- KernelArgumentBlock& args);
+__LIBC_HIDDEN__ void __libc_init_global_stack_chk_guard(KernelArgumentBlock& args);
__LIBC_HIDDEN__ void __libc_init_malloc(libc_globals* globals);
+__LIBC_HIDDEN__ void __libc_init_setjmp_cookie(libc_globals* globals, KernelArgumentBlock& args);
+__LIBC_HIDDEN__ void __libc_init_vdso(libc_globals* globals, KernelArgumentBlock& args);
+
#endif
diff --git a/libc/private/bionic_tls.h b/libc/private/bionic_tls.h
index f13615d..c61e2ff 100644
--- a/libc/private/bionic_tls.h
+++ b/libc/private/bionic_tls.h
@@ -30,7 +30,7 @@
#define __BIONIC_PRIVATE_BIONIC_TLS_H_
#include <sys/cdefs.h>
-#include <sys/limits.h>
+
#include "bionic_macros.h"
#include "__get_tls.h"
@@ -117,7 +117,7 @@
#if defined(__cplusplus)
class KernelArgumentBlock;
-extern __LIBC_HIDDEN__ void __libc_init_main_thread(KernelArgumentBlock&);
+extern void __libc_init_main_thread(KernelArgumentBlock&);
#endif
#endif /* __BIONIC_PRIVATE_BIONIC_TLS_H_ */
diff --git a/tests/Android.mk b/tests/Android.mk
index 0db63d9..956b76f 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -78,6 +78,7 @@
setjmp_test.cpp \
signal_test.cpp \
stack_protector_test.cpp \
+ stack_protector_test_helper.cpp \
stack_unwinding_test.cpp \
stdatomic_test.cpp \
stdint_test.cpp \
diff --git a/tests/stack_protector_test.cpp b/tests/stack_protector_test.cpp
index 22285d1..5f5a241 100644
--- a/tests/stack_protector_test.cpp
+++ b/tests/stack_protector_test.cpp
@@ -27,108 +27,83 @@
#include <unistd.h>
#include <set>
-extern "C" pid_t gettid();
+#include "private/bionic_tls.h"
-// For x86, bionic and glibc have per-thread stack guard values (all identical).
-#if defined(__i386__)
-static uint32_t GetGuardFromTls() {
- uint32_t guard;
- asm ("mov %%gs:0x14, %0": "=d" (guard));
- return guard;
-}
+extern "C" pid_t gettid(); // glibc defines this but doesn't declare it anywhere.
+
+#if defined(__BIONIC__)
+extern uintptr_t __stack_chk_guard;
+#endif
struct stack_protector_checker {
std::set<pid_t> tids;
- std::set<uint32_t> guards;
+ std::set<void*> guards;
void Check() {
pid_t tid = gettid();
- uint32_t guard = GetGuardFromTls();
+ void* guard = __get_tls()[TLS_SLOT_STACK_GUARD];
- printf("[thread %d] %%gs:0x14 = 0x%08x\n", tid, guard);
+ printf("[thread %d] TLS stack guard = %p\n", tid, guard);
// Duplicate tid. gettid(2) bug? Seeing this would be very upsetting.
ASSERT_TRUE(tids.find(tid) == tids.end());
- // Uninitialized guard. Our bug. Note this is potentially flaky; we _could_ get
- // four random zero bytes, but it should be vanishingly unlikely.
- ASSERT_NE(guard, 0U);
+ // Uninitialized guard. Our bug. Note this is potentially flaky; we _could_
+ // get four random zero bytes, but it should be vanishingly unlikely.
+ ASSERT_NE(guard, nullptr);
+
+#if defined(__BIONIC__)
+ // bionic always has the global too.
+ ASSERT_EQ(__stack_chk_guard, reinterpret_cast<uintptr_t>(guard));
+#endif
tids.insert(tid);
guards.insert(guard);
}
};
-static void* ThreadGuardHelper(void* arg) {
- stack_protector_checker* checker = reinterpret_cast<stack_protector_checker*>(arg);
- checker->Check();
- return NULL;
-}
-#endif // __i386__
-
TEST(stack_protector, same_guard_per_thread) {
-#if defined(__i386__)
+ // Everyone has the TLS slot set, even if their stack protector
+ // implementation doesn't yet use it.
stack_protector_checker checker;
- size_t thread_count = 10;
- for (size_t i = 0; i < thread_count; ++i) {
+
+ // Check the main thread.
+ ASSERT_EQ(getpid(), gettid()); // We are the main thread, right?
+ checker.Check();
+
+ size_t thread_count = 9;
+ for (size_t i = 1; i < thread_count; ++i) {
pthread_t t;
- ASSERT_EQ(0, pthread_create(&t, NULL, ThreadGuardHelper, &checker));
+ ASSERT_EQ(0, pthread_create(&t, NULL, [](void* arg) -> void* {
+ stack_protector_checker* checker = reinterpret_cast<stack_protector_checker*>(arg);
+ checker->Check();
+ return nullptr;
+ }, &checker));
void* result;
ASSERT_EQ(0, pthread_join(t, &result));
ASSERT_EQ(NULL, result);
}
ASSERT_EQ(thread_count, checker.tids.size());
- // bionic and glibc use the same guard for every thread.
+ // Both bionic and glibc use the same guard for every thread.
ASSERT_EQ(1U, checker.guards.size());
-#else // __i386__
- GTEST_LOG_(INFO) << "This test does nothing.\n";
-#endif // __i386__
}
-// For ARM and MIPS, glibc has a global stack check guard value.
-#if defined(__BIONIC__) || defined(__arm__) || defined(__mips__)
-#define TEST_STACK_CHK_GUARD
-
-// Bionic has the global for x86 too, to support binaries that can run on
-// Android releases that didn't implement the TLS guard value.
-extern "C" uintptr_t __stack_chk_guard;
-
-/*
- * When this function returns, the stack canary will be inconsistent
- * with the previous value, which will generate a call to __stack_chk_fail(),
- * eventually resulting in a SIGABRT.
- *
- * This must be marked with "__attribute__ ((noinline))", to ensure the
- * compiler generates the proper stack guards around this function.
- */
-static char* dummy_buf;
-
-__attribute__ ((noinline))
-static void do_modify_stack_chk_guard() {
- char buf[128];
- // Store local array's address to global variable to force compiler to generate stack guards.
- dummy_buf = buf;
- __stack_chk_guard = 0x12345678;
-}
-
-#endif
-
TEST(stack_protector, global_guard) {
-#if defined(TEST_STACK_CHK_GUARD)
+#if defined(__BIONIC__)
+ // Bionic always has a global, even if it's using TLS.
ASSERT_NE(0, gettid());
ASSERT_NE(0U, __stack_chk_guard);
-#else // TEST_STACK_CHK_GUARD
- GTEST_LOG_(INFO) << "This test does nothing.\n";
-#endif // TEST_STACK_CHK_GUARD
+#else
+ GTEST_LOG_(INFO) << "glibc doesn't have a global __stack_chk_guard.\n";
+#endif
}
class stack_protector_DeathTest : public BionicDeathTest {};
TEST_F(stack_protector_DeathTest, modify_stack_protector) {
-#if defined(TEST_STACK_CHK_GUARD)
- ASSERT_EXIT(do_modify_stack_chk_guard(), testing::KilledBySignal(SIGABRT), "");
-#else // TEST_STACK_CHK_GUARD
- GTEST_LOG_(INFO) << "This test does nothing.\n";
-#endif // TEST_STACK_CHK_GUARD
+ // In another file to prevent inlining, which removes stack protection.
+ extern void modify_stack_protector_test();
+ ASSERT_EXIT(modify_stack_protector_test(),
+ testing::KilledBySignal(SIGABRT), "stack corruption detected");
}
diff --git a/tests/stack_protector_test_helper.cpp b/tests/stack_protector_test_helper.cpp
new file mode 100644
index 0000000..34f3c77
--- /dev/null
+++ b/tests/stack_protector_test_helper.cpp
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2012 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// Deliberately overwrite the stack canary.
+__attribute__((noinline)) void modify_stack_protector_test() {
+ char buf[128];
+ // We can't use memset here because it's fortified, and we want to test
+ // the line of defense *after* that.
+ char* p = buf;
+ while ((p - buf) < static_cast<int>(sizeof(buf) + sizeof(void*))) *p++ = '\0';
+}
