From 416d7ddaff0946d480b6aa945a741b3eeaca5569 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Mon, 18 Aug 2014 17:28:32 -0700 Subject: [PATCH] Add GNU-compatible strerror_r. We already had the POSIX strerror_r, but some third-party code defines _GNU_SOURCE and expects to get the GNU strerror_r instead. This exposed a bug in the libc internal logging functions where unlike their standard brethren they wouldn't return the number of bytes they'd have liked to have written. Bug: 16243479 Change-Id: I1745752ccbdc569646d34f5071f6df2be066d5f4 --- libc/bionic/libc_logging.cpp | 14 ++++--- libc/bionic/strerror_r.cpp | 13 +++++- libc/include/string.h | 8 +++- tests/Android.mk | 1 + tests/libc_logging_test.cpp | 12 ++++++ tests/string_posix_strerror_r_test.cpp | 57 ++++++++++++++++++++++++++ tests/string_test.cpp | 30 +++++++++----- 7 files changed, 115 insertions(+), 20 deletions(-) create mode 100644 tests/string_posix_strerror_r_test.cpp diff --git a/libc/bionic/libc_logging.cpp b/libc/bionic/libc_logging.cpp index 565552665..49a37628f 100644 --- a/libc/bionic/libc_logging.cpp +++ b/libc/bionic/libc_logging.cpp @@ -75,10 +75,12 @@ struct BufferOutputStream { len = strlen(data); } + total += len; + while (len > 0) { int avail = end_ - pos_; if (avail == 0) { - break; + return; } if (avail > len) { avail = len; @@ -87,11 +89,10 @@ struct BufferOutputStream { pos_ += avail; pos_[0] = '\0'; len -= avail; - total += avail; } } - int total; + size_t total; private: char* buffer_; @@ -109,18 +110,19 @@ struct FdOutputStream { len = strlen(data); } + total += len; + while (len > 0) { int rc = TEMP_FAILURE_RETRY(write(fd_, data, len)); if (rc == -1) { - break; + return; } data += rc; len -= rc; - total += rc; } } - int total; + size_t total; private: int fd_; diff --git a/libc/bionic/strerror_r.cpp b/libc/bionic/strerror_r.cpp index 1e57cc0b3..d419fb109 100644 --- a/libc/bionic/strerror_r.cpp +++ b/libc/bionic/strerror_r.cpp @@ -1,11 +1,16 @@ /* $OpenBSD: strerror_r.c,v 1.6 2005/08/08 08:05:37 espie Exp $ */ /* Public Domain */ +// G++ automatically defines _GNU_SOURCE, which then means that +// gives us the GNU variant. +#undef _GNU_SOURCE + +#include + #include #include #include #include -#include #include "private/ErrnoRestorer.h" #include "private/libc_logging.h" @@ -62,6 +67,12 @@ int strerror_r(int error_number, char* buf, size_t buf_len) { return 0; } +extern "C" char* __gnu_strerror_r(int error_number, char* buf, size_t buf_len) { + ErrnoRestorer errno_restorer; // The glibc strerror_r doesn't set errno if it truncates... + strerror_r(error_number, buf, buf_len); + return buf; // ...and just returns whatever fit. +} + extern "C" __LIBC_HIDDEN__ const char* __strsignal(int signal_number, char* buf, size_t buf_len) { const char* signal_name = __strsignal_lookup(signal_number); if (signal_name != NULL) { diff --git a/libc/include/string.h b/libc/include/string.h index 0a1d653b8..b0643af6c 100644 --- a/libc/include/string.h +++ b/libc/include/string.h @@ -67,8 +67,12 @@ extern char* strcasestr(const char *haystack, const char *needle) __purefunc; extern char* strtok(char* __restrict, const char* __restrict); extern char* strtok_r(char* __restrict, const char* __restrict, char** __restrict); -extern char* strerror(int); -extern int strerror_r(int errnum, char *buf, size_t n); +extern char* strerror(int); +#if defined(__USE_GNU) +extern char* strerror_r(int, char*, size_t) __RENAME(__gnu_strerror_r); +#else /* POSIX */ +extern int strerror_r(int, char*, size_t); +#endif extern size_t strnlen(const char *, size_t) __purefunc; extern char* strncat(char* __restrict, const char* __restrict, size_t); diff --git a/tests/Android.mk b/tests/Android.mk index 5179bfa09..26014aca3 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -95,6 +95,7 @@ libBionicStandardTests_src_files := \ stdio_ext_test.cpp \ stdlib_test.cpp \ string_test.cpp \ + string_posix_strerror_r_test.cpp \ strings_test.cpp \ stubs_test.cpp \ sstream_test.cpp \ diff --git a/tests/libc_logging_test.cpp b/tests/libc_logging_test.cpp index 950161e78..d4ceded45 100644 --- a/tests/libc_logging_test.cpp +++ b/tests/libc_logging_test.cpp @@ -176,3 +176,15 @@ TEST(libc_logging, lld_LLONG_MIN) { GTEST_LOG_(INFO) << "This test does nothing.\n"; #endif // __BIONIC__ } + +TEST(libc_logging, buffer_overrun) { +#if defined(__BIONIC__) + char buf[BUFSIZ]; + ASSERT_EQ(11, __libc_format_buffer(buf, sizeof(buf), "hello %s", "world")); + EXPECT_STREQ("hello world", buf); + ASSERT_EQ(11, __libc_format_buffer(buf, 8, "hello %s", "world")); + EXPECT_STREQ("hello w", buf); +#else // __BIONIC__ + GTEST_LOG_(INFO) << "This test does nothing.\n"; +#endif // __BIONIC__ +} diff --git a/tests/string_posix_strerror_r_test.cpp b/tests/string_posix_strerror_r_test.cpp new file mode 100644 index 000000000..09cebfecc --- /dev/null +++ b/tests/string_posix_strerror_r_test.cpp @@ -0,0 +1,57 @@ +/* + * 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. + */ + +#undef _GNU_SOURCE + +// Old versions of glibc (like our current host prebuilt sysroot one) have +// headers that don't work if you #undef _GNU_SOURCE, which makes it +// impossible to build this test. +#include + +#if !defined(__GLIBC__) +#include + +#include +#include + +TEST(string, posix_strerror_r) { + char buf[256]; + + // Valid. + ASSERT_EQ(0, strerror_r(0, buf, sizeof(buf))); + ASSERT_STREQ("Success", buf); + ASSERT_EQ(0, strerror_r(1, buf, sizeof(buf))); + ASSERT_STREQ("Operation not permitted", buf); + + // Invalid. + ASSERT_EQ(0, strerror_r(-1, buf, sizeof(buf))); + ASSERT_STREQ("Unknown error -1", buf); + ASSERT_EQ(0, strerror_r(1234, buf, sizeof(buf))); + ASSERT_STREQ("Unknown error 1234", buf); + + // Buffer too small. + errno = 0; + memset(buf, 0, sizeof(buf)); + ASSERT_EQ(-1, strerror_r(4567, buf, 2)); + ASSERT_STREQ("U", buf); + // The POSIX strerror_r sets errno to ERANGE (the GNU one doesn't). + ASSERT_EQ(ERANGE, errno); +} +#else +# if __GLIBC_PREREQ(2, 15) +# error this test should work now +# endif +#endif diff --git a/tests/string_test.cpp b/tests/string_test.cpp index a3c6abb7d..7db8e2140 100644 --- a/tests/string_test.cpp +++ b/tests/string_test.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#define _GNU_SOURCE 1 + #include #include @@ -72,28 +74,34 @@ TEST(string, strerror_concurrent) { #endif // __BIONIC__ } -TEST(string, strerror_r) { -#if defined(__BIONIC__) // glibc's strerror_r doesn't even have the same signature as the POSIX one. +TEST(string, gnu_strerror_r) { char buf[256]; + // Note that glibc doesn't necessarily write into the buffer. + // Valid. - ASSERT_EQ(0, strerror_r(0, buf, sizeof(buf))); + ASSERT_STREQ("Success", strerror_r(0, buf, sizeof(buf))); +#if defined(__BIONIC__) ASSERT_STREQ("Success", buf); - ASSERT_EQ(0, strerror_r(1, buf, sizeof(buf))); +#endif + ASSERT_STREQ("Operation not permitted", strerror_r(1, buf, sizeof(buf))); +#if defined(__BIONIC__) ASSERT_STREQ("Operation not permitted", buf); +#endif // Invalid. - ASSERT_EQ(0, strerror_r(-1, buf, sizeof(buf))); + ASSERT_STREQ("Unknown error -1", strerror_r(-1, buf, sizeof(buf))); ASSERT_STREQ("Unknown error -1", buf); - ASSERT_EQ(0, strerror_r(1234, buf, sizeof(buf))); + ASSERT_STREQ("Unknown error 1234", strerror_r(1234, buf, sizeof(buf))); ASSERT_STREQ("Unknown error 1234", buf); // Buffer too small. - ASSERT_EQ(-1, strerror_r(0, buf, 2)); - ASSERT_EQ(ERANGE, errno); -#else // __BIONIC__ - GTEST_LOG_(INFO) << "This test does nothing.\n"; -#endif // __BIONIC__ + errno = 0; + memset(buf, 0, sizeof(buf)); + ASSERT_EQ(buf, strerror_r(4567, buf, 2)); + ASSERT_STREQ("U", buf); + // The GNU strerror_r doesn't set errno (the POSIX one sets it to ERANGE). + ASSERT_EQ(0, errno); } TEST(string, strsignal) {