From ad88a0863110798cef5169dcf917e18b967a7cf6 Mon Sep 17 00:00:00 2001
From: Elliott Hughes <enh@google.com>
Date: Wed, 24 Oct 2012 18:37:21 -0700
Subject: [PATCH] Per-thread -fstack-protector guards for x86.

Based on a pair of patches from Intel:

  https://android-review.googlesource.com/#/c/43909/
  https://android-review.googlesource.com/#/c/44903/

For x86, this patch supports _both_ the global that ARM/MIPS use
and the per-thread TLS entry (%gs:20) that GCC uses by default. This
lets us support binaries built with any x86 toolchain (right now,
the NDK is emitting x86 code that uses the global).

I've also extended the original tests to cover ARM/MIPS too, and
be a little more thorough for x86.

Change-Id: I02f279a80c6b626aecad449771dec91df235ad01
---
 libc/Android.mk                |   2 +-
 libc/bionic/pthread.c          |   7 +-
 libc/bionic/ssp.c              | 100 ----------------------------
 libc/bionic/ssp.cpp            |  81 +++++++++++++++++++++++
 libc/private/bionic_ssp.h      |  76 +++++++++++++++++++++
 libc/private/bionic_tls.h      |  15 ++---
 tests/Android.mk               |  10 +++
 tests/dlopen_test.cpp          |   2 +-
 tests/stack_protector_test.cpp | 117 +++++++++++++++++++++++++++++++++
 tests/string_test.cpp          |   8 ++-
 10 files changed, 301 insertions(+), 117 deletions(-)
 delete mode 100644 libc/bionic/ssp.c
 create mode 100644 libc/bionic/ssp.cpp
 create mode 100644 libc/private/bionic_ssp.h
 create mode 100644 tests/stack_protector_test.cpp

diff --git a/libc/Android.mk b/libc/Android.mk
index 780f478b4..a037e7a1d 100644
--- a/libc/Android.mk
+++ b/libc/Android.mk
@@ -740,7 +740,7 @@ WITH_MALLOC_CHECK_LIBC_A := $(strip $(WITH_MALLOC_CHECK_LIBC_A))
 
 include $(CLEAR_VARS)
 
-LOCAL_SRC_FILES := bionic/ssp.c
+LOCAL_SRC_FILES := bionic/ssp.cpp
 LOCAL_CFLAGS := $(libc_common_cflags) -fno-stack-protector
 LOCAL_C_INCLUDES := $(libc_common_c_includes)
 LOCAL_MODULE := libbionic_ssp
diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c
index 719bc83b5..7c22b45b2 100644
--- a/libc/bionic/pthread.c
+++ b/libc/bionic/pthread.c
@@ -48,6 +48,7 @@
 #include "bionic_atomic_inline.h"
 #include "bionic_futex.h"
 #include "bionic_pthread.h"
+#include "bionic_ssp.h"
 #include "bionic_tls.h"
 #include "pthread_internal.h"
 #include "thread_private.h"
@@ -171,12 +172,14 @@ void  __init_tls(void** tls, void* thread) {
     tls[i] = NULL;
   }
 
-  // Slot 0 must point to the tls area, this is required by the implementation
-  // of the x86 Linux kernel thread-local-storage.
+  // Slot 0 must point to itself. The x86 Linux kernel reads the TLS from %fs:0.
   tls[TLS_SLOT_SELF]      = (void*) tls;
   tls[TLS_SLOT_THREAD_ID] = thread;
 
+  // Stack guard generation may make system calls, and those system calls may fail.
+  // If they do, they'll try to set errno, so we can only do this after calling __set_tls.
   __set_tls((void*) tls);
+  tls[TLS_SLOT_STACK_GUARD] = __generate_stack_chk_guard();
 }
 
 
diff --git a/libc/bionic/ssp.c b/libc/bionic/ssp.c
deleted file mode 100644
index f83b2a447..000000000
--- a/libc/bionic/ssp.c
+++ /dev/null
@@ -1,100 +0,0 @@
-/*
- * Copyright (C) 2008 The Android Open Source Project
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- *  * Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- *  * Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in
- *    the documentation and/or other materials provided with the
- *    distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
- * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
- * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
- * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
- * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
- * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
- * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
- * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
- * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-#include <stdio.h>
-#include <stdlib.h>
-#include <signal.h>
-#include <string.h>
-#include <unistd.h>
-#include <fcntl.h>
-#include <ctype.h>
-#include "logd.h"
-
-void *__stack_chk_guard = 0;
-
-/* Initialize the canary with a random value from /dev/urandom.
- * If that fails, use the "terminator canary". */
-static void __attribute__ ((constructor))
-__guard_setup(void)
-{
-    int fd;
-
-    fd = open("/dev/urandom", O_RDONLY);
-    if (fd != -1) {
-        ssize_t len = read(fd, &__stack_chk_guard,
-                           sizeof(__stack_chk_guard));
-        close(fd);
-        if (len == sizeof(__stack_chk_guard))
-            return;
-    }
-
-    /* If that failed, switch to 'terminator canary' */
-    ((unsigned char *)&__stack_chk_guard)[0] = 0;
-    ((unsigned char *)&__stack_chk_guard)[1] = 0;
-    ((unsigned char *)&__stack_chk_guard)[2] = '\n';
-    ((unsigned char *)&__stack_chk_guard)[3] = 255;
-}
-
-/* This is the crash handler.
- * Does a best effort at logging and calls _exit to terminate
- * the process immediately (without atexit handlers, etc.) */
-void __stack_chk_fail(void)
-{
-    struct sigaction sa;
-    sigset_t sigmask;
-    static const char message[] = "stack corruption detected: aborted";
-    char path[PATH_MAX];
-    int count;
-
-    /* Immediately block all (but SIGABRT) signal handlers from running code */
-    sigfillset(&sigmask);
-    sigdelset(&sigmask, SIGABRT);
-    sigprocmask(SIG_BLOCK, &sigmask, NULL);
-
-    /* Use /proc/self/exe link to obtain the program name for logging
-     * purposes. If it's not available, we set it to "<unknown>" */
-    if ((count = readlink("/proc/self/exe", path, sizeof(path) - 1)) == -1) {
-        strlcpy(path, "<unknown>", sizeof(path));
-    } else {
-        path[count] = '\0';
-    }
-
-    /* Do a best effort at logging. This ends up calling writev(2) */
-    __libc_android_log_print(ANDROID_LOG_FATAL, path, message);
-
-    /* Make sure there is no default action for SIGABRT */
-    bzero(&sa, sizeof(struct sigaction));
-    sigemptyset(&sa.sa_mask);
-    sa.sa_flags = 0;
-    sa.sa_handler = SIG_DFL;
-    sigaction(SIGABRT, &sa, NULL);
-
-    /* Terminate the process and exit immediately */
-    kill(getpid(), SIGABRT);
-
-    _exit(127);
-}
diff --git a/libc/bionic/ssp.cpp b/libc/bionic/ssp.cpp
new file mode 100644
index 000000000..fdf88328b
--- /dev/null
+++ b/libc/bionic/ssp.cpp
@@ -0,0 +1,81 @@
+/*
+ * Copyright (C) 2008 The Android Open Source Project
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *  * Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ *  * Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in
+ *    the documentation and/or other materials provided with the
+ *    distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+ * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+ * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <ctype.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "bionic_ssp.h"
+#include "logd.h"
+
+void* __stack_chk_guard = NULL;
+
+static void __attribute__((constructor)) __init_stack_check_guard() {
+  __stack_chk_guard = __generate_stack_chk_guard();
+}
+
+// This is the crash handler.
+// Does a best effort at logging and calls _exit to terminate
+// the process immediately (without atexit handlers, etc.).
+void __stack_chk_fail() {
+  // Immediately block all (but SIGABRT) signal handlers from running code.
+  sigset_t sigmask;
+  sigfillset(&sigmask);
+  sigdelset(&sigmask, SIGABRT);
+  sigprocmask(SIG_BLOCK, &sigmask, NULL);
+
+  // Use /proc/self/exe link to obtain the program name for logging
+  // purposes. If it's not available, we set it to "<unknown>".
+  char path[PATH_MAX];
+  int count;
+  if ((count = readlink("/proc/self/exe", path, sizeof(path) - 1)) == -1) {
+    strlcpy(path, "<unknown>", sizeof(path));
+  } else {
+    path[count] = '\0';
+  }
+
+  // Do a best effort at logging. This ends up calling writev(2).
+  __libc_android_log_print(ANDROID_LOG_FATAL, path, "stack corruption detected: aborted");
+
+  // Make sure there is no default action for SIGABRT.
+  struct sigaction sa;
+  memset(&sa, 0, sizeof(sa));
+  sigemptyset(&sa.sa_mask);
+  sa.sa_flags = 0;
+  sa.sa_handler = SIG_DFL;
+  sigaction(SIGABRT, &sa, NULL);
+
+  // Terminate the process and exit immediately.
+  kill(getpid(), SIGABRT);
+
+  _exit(127);
+}
diff --git a/libc/private/bionic_ssp.h b/libc/private/bionic_ssp.h
new file mode 100644
index 000000000..697216c18
--- /dev/null
+++ b/libc/private/bionic_ssp.h
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2012 The Android Open Source Project
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *  * Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ *  * Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in
+ *    the documentation and/or other materials provided with the
+ *    distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+ * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+ * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#ifndef _PRIVATE_SSP_H
+#define _PRIVATE_SSP_H
+
+#include <errno.h>
+#include <sys/cdefs.h>
+
+__BEGIN_DECLS
+
+/** WARNING WARNING WARNING
+ **
+ ** This header file is *NOT* part of the public Bionic ABI/API
+ ** and should not be used/included by user-serviceable parts of
+ ** the system (e.g. applications).
+ **/
+
+/* GCC uses this on ARM and MIPS. */
+extern void* __stack_chk_guard;
+
+/* GCC calls this if a stack guard check fails. */
+extern void __stack_chk_fail();
+
+__inline__ static void* __attribute__((always_inline)) __generate_stack_chk_guard(void) {
+  union {
+    uintptr_t value;
+    char bytes[sizeof(uintptr_t)];
+  } u;
+
+  /* Try pulling random bytes from /dev/urandom. */
+  int fd = TEMP_FAILURE_RETRY(open("/dev/urandom", O_RDONLY));
+  if (fd != -1) {
+    ssize_t byte_count = TEMP_FAILURE_RETRY(read(fd, &u.bytes, sizeof(u)));
+    close(fd);
+    if (byte_count == sizeof(u)) {
+      return (void*) u.value;
+    }
+  }
+
+  /* If that failed, switch to 'terminator canary'. */
+  u.bytes[0] = 0;
+  u.bytes[1] = 0;
+  u.bytes[2] = '\n';
+  u.bytes[3] = 255;
+  return (void*) u.value;
+}
+
+__END_DECLS
+
+#endif
diff --git a/libc/private/bionic_tls.h b/libc/private/bionic_tls.h
index a626d2172..f661ccf24 100644
--- a/libc/private/bionic_tls.h
+++ b/libc/private/bionic_tls.h
@@ -43,24 +43,19 @@ __BEGIN_DECLS
  ** pre-allocated slot directly for performance reason).
  **/
 
-/* maximum number of elements in the TLS array */
+/* Maximum number of elements in the TLS array. */
 #define BIONIC_TLS_SLOTS            64
 
-/* note that slot 0, called TLS_SLOT_SELF must point to itself.
- * this is required to implement thread-local storage with the x86
- * Linux kernel, that reads the TLS from fs:[0], where 'fs' is a
- * thread-specific segment descriptor...
- */
-
-/* Well-known TLS slots. */
-#define TLS_SLOT_SELF               0
+/* Well-known TLS slots. What data goes in which slot is arbitrary unless otherwise noted. */
+#define TLS_SLOT_SELF               0  /* The kernel requires this specific slot for x86. */
 #define TLS_SLOT_THREAD_ID          1
 #define TLS_SLOT_ERRNO              2
 
 #define TLS_SLOT_OPENGL_API         3
 #define TLS_SLOT_OPENGL             4
 
-#define TLS_SLOT_DLERROR            5
+#define TLS_SLOT_STACK_GUARD        5  /* GCC requires this specific slot for x86. */
+#define TLS_SLOT_DLERROR            6
 
 #define TLS_SLOT_MAX_WELL_KNOWN     TLS_SLOT_DLERROR
 
diff --git a/tests/Android.mk b/tests/Android.mk
index 259aced9e..e38aaf9a9 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -18,10 +18,17 @@ ifneq ($(BUILD_TINY_ANDROID), true)
 
 LOCAL_PATH := $(call my-dir)
 
+test_c_flags = \
+    -fstack-protector \
+    -g \
+    -Wall -Wextra \
+    -Werror \
+
 test_src_files = \
     getcwd_test.cpp \
     pthread_test.cpp \
     regex_test.cpp \
+    stack_protector_test.cpp \
     stdio_test.cpp \
     stdlib_test.cpp \
     string_test.cpp \
@@ -36,6 +43,7 @@ test_dynamic_src_files = \
 include $(CLEAR_VARS)
 LOCAL_MODULE := bionic-unit-tests
 LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
+LOCAL_CFLAGS += $(test_c_flags)
 LOCAL_LDFLAGS += $(test_dynamic_ldflags)
 LOCAL_SHARED_LIBRARIES += libdl
 LOCAL_SRC_FILES := $(test_src_files) $(test_dynamic_src_files)
@@ -46,6 +54,7 @@ include $(BUILD_NATIVE_TEST)
 include $(CLEAR_VARS)
 LOCAL_MODULE := bionic-unit-tests-static
 LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
+LOCAL_CFLAGS += $(test_c_flags)
 LOCAL_FORCE_STATIC_EXECUTABLE := true
 LOCAL_SRC_FILES := $(test_src_files)
 LOCAL_STATIC_LIBRARIES += libstlport_static libstdc++ libm libc
@@ -59,6 +68,7 @@ ifeq ($(HOST_OS)-$(HOST_ARCH),linux-x86)
 include $(CLEAR_VARS)
 LOCAL_MODULE := bionic-unit-tests-glibc
 LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
+LOCAL_CFLAGS += $(test_c_flags)
 LOCAL_LDFLAGS += -lpthread -ldl
 LOCAL_LDFLAGS += $(test_dynamic_ldflags)
 LOCAL_SRC_FILES := $(test_src_files) $(test_dynamic_src_files)
diff --git a/tests/dlopen_test.cpp b/tests/dlopen_test.cpp
index 5b5c7f68e..d38d8c573 100644
--- a/tests/dlopen_test.cpp
+++ b/tests/dlopen_test.cpp
@@ -58,7 +58,7 @@ TEST(dlopen, dlopen_failure) {
 #endif
 }
 
-static void* ConcurrentDlErrorFn(void* arg) {
+static void* ConcurrentDlErrorFn(void*) {
   dlopen("/child/thread", RTLD_NOW);
   return reinterpret_cast<void*>(strdup(dlerror()));
 }
diff --git a/tests/stack_protector_test.cpp b/tests/stack_protector_test.cpp
new file mode 100644
index 000000000..9d86506ac
--- /dev/null
+++ b/tests/stack_protector_test.cpp
@@ -0,0 +1,117 @@
+/*
+ * 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.
+ */
+
+/*
+ * Contributed by: Intel Corporation
+ */
+
+#include <gtest/gtest.h>
+
+#include <pthread.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+#include <set>
+
+#ifdef __GLIBC__
+
+// glibc doesn't expose gettid(2).
+pid_t gettid() { return syscall(__NR_gettid); }
+
+#endif
+
+#ifdef __i386__
+
+// For x86, bionic and glibc have per-thread stack guard values.
+
+static uint32_t GetGuardFromTls() {
+  uint32_t guard;
+  asm ("mov %%gs:0x14, %0": "=d" (guard));
+  return guard;
+}
+
+struct stack_protector_checker {
+  std::set<pid_t> tids;
+  std::set<uint32_t> guards;
+
+  void Check() {
+    pid_t tid = gettid();
+    uint32_t guard = GetGuardFromTls();
+
+    printf("[thread %d] %%gs:0x14 = 0x%08x\n", tid, guard);
+
+    // Duplicate tid. gettid(2) bug? Seeing this would be very upsetting.
+    ASSERT_TRUE(tids.find(tid) == tids.end());
+#ifdef __GLIBC__
+    // glibc uses the same guard for every thread. bionic uses a different guard for each one.
+#else
+    // Duplicate guard. Our bug. Note this is potentially flaky; we _could_ get the
+    // same guard for two threads, but it should be vanishingly unlikely.
+    ASSERT_TRUE(guards.find(guard) == guards.end());
+#endif
+    // 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);
+
+    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;
+}
+
+TEST(stack_protector, guard_per_thread) {
+  stack_protector_checker checker;
+  size_t thread_count = 10;
+  for (size_t i = 0; i < thread_count; ++i) {
+    pthread_t t;
+    ASSERT_EQ(0, pthread_create(&t, NULL, ThreadGuardHelper, &checker));
+    void* result;
+    ASSERT_EQ(0, pthread_join(t, &result));
+    ASSERT_EQ(NULL, result);
+  }
+  ASSERT_EQ(thread_count, checker.tids.size());
+
+  // glibc uses the same guard for every thread. bionic uses a different guard for each one.
+#ifdef __BIONIC__
+  ASSERT_EQ(thread_count, checker.guards.size());
+#else
+  ASSERT_EQ(1U, checker.guards.size());
+#endif
+}
+
+#endif
+
+#if defined(__BIONIC__) || defined(__arm__) || defined(__mips__)
+
+// For ARM and MIPS, glibc has a global stack check guard value.
+
+// 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" void* __stack_chk_guard;
+
+TEST(stack_protector, global_guard) {
+  ASSERT_NE(0, gettid());
+  ASSERT_NE(0U, reinterpret_cast<uintptr_t>(__stack_chk_guard));
+}
+
+#endif
diff --git a/tests/string_test.cpp b/tests/string_test.cpp
index 47469d853..472aacb0e 100644
--- a/tests/string_test.cpp
+++ b/tests/string_test.cpp
@@ -29,12 +29,13 @@ TEST(string, strerror) {
   ASSERT_STREQ("Unknown error 1234", strerror(1234));
 }
 
-static void* ConcurrentStrErrorFn(void* arg) {
+#if __BIONIC__ // glibc's strerror isn't thread safe, only its strsignal.
+
+static void* ConcurrentStrErrorFn(void*) {
   bool equal = (strcmp("Unknown error 2002", strerror(2002)) == 0);
   return reinterpret_cast<void*>(equal);
 }
 
-#if __BIONIC__ // glibc's strerror isn't thread safe, only its strsignal.
 TEST(string, strerror_concurrent) {
   const char* strerror1001 = strerror(1001);
   ASSERT_STREQ("Unknown error 1001", strerror1001);
@@ -47,6 +48,7 @@ TEST(string, strerror_concurrent) {
 
   ASSERT_STREQ("Unknown error 1001", strerror1001);
 }
+
 #endif
 
 #if __BIONIC__ // glibc's strerror_r doesn't even have the same signature as the POSIX one.
@@ -88,7 +90,7 @@ TEST(string, strsignal) {
   ASSERT_STREQ("Unknown signal 1234", strsignal(1234)); // Too large.
 }
 
-static void* ConcurrentStrSignalFn(void* arg) {
+static void* ConcurrentStrSignalFn(void*) {
   bool equal = (strcmp("Unknown signal 2002", strsignal(2002)) == 0);
   return reinterpret_cast<void*>(equal);
 }