From 05de1ba38eea7675c7af547c6e608417fd3ca1bc Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 12 Feb 2019 11:26:23 -0800 Subject: [PATCH] liblog: simplify socket_local_client() and always use CLOEXEC socket_local_client() was copied from libcutils, but we only need a small subset of its functionality. We instead create our own version with just the needed parts. Importantly, use CLOEXEC in this new function and other places where it was missing previously. Test: logging works, liblog-unit-tests Change-Id: Ifb929227af67bafa13e391eab92358d9f6fe6450 --- liblog/logd_reader.cpp | 157 ++++-------------------------- liblog/tests/Android.bp | 1 + liblog/tests/libc_test.cpp | 2 +- liblog/tests/liblog_benchmark.cpp | 14 +-- liblog/tests/liblog_test.cpp | 4 +- liblog/tests/log_radio_test.cpp | 2 +- liblog/tests/log_system_test.cpp | 2 +- 7 files changed, 32 insertions(+), 150 deletions(-) diff --git a/liblog/logd_reader.cpp b/liblog/logd_reader.cpp index 2f0af4a2e..b7ba7826d 100644 --- a/liblog/logd_reader.cpp +++ b/liblog/logd_reader.cpp @@ -100,148 +100,29 @@ static int logdAvailable(log_id_t logId) { return -EBADF; } -/* Private copy of ../libcutils/socket_local_client.c prevent library loops */ +// Connects to /dev/socket/ and returns the associated fd or returns -1 on error. +// O_CLOEXEC is always set. +static int socket_local_client(const std::string& name, int type) { + sockaddr_un addr = {.sun_family = AF_LOCAL}; -#if defined(_WIN32) - -LIBLOG_WEAK int socket_local_client(const char* name, int namespaceId, int type) { - errno = ENOSYS; - return -ENOSYS; -} - -#else /* !_WIN32 */ - -#include -#include -#include -#include - -/* Private copy of ../libcutils/socket_local.h prevent library loops */ -#define FILESYSTEM_SOCKET_PREFIX "/tmp/" -#define ANDROID_RESERVED_SOCKET_PREFIX "/dev/socket/" -/* End of ../libcutils/socket_local.h */ - -#define LISTEN_BACKLOG 4 - -/* Documented in header file. */ -LIBLOG_WEAK int socket_make_sockaddr_un(const char* name, int namespaceId, - struct sockaddr_un* p_addr, socklen_t* alen) { - memset(p_addr, 0, sizeof(*p_addr)); - size_t namelen; - - switch (namespaceId) { - case ANDROID_SOCKET_NAMESPACE_ABSTRACT: -#if defined(__linux__) - namelen = strlen(name); - - /* Test with length +1 for the *initial* '\0'. */ - if ((namelen + 1) > sizeof(p_addr->sun_path)) { - goto error; - } - - /* - * Note: The path in this case is *not* supposed to be - * '\0'-terminated. ("man 7 unix" for the gory details.) - */ - - p_addr->sun_path[0] = 0; - memcpy(p_addr->sun_path + 1, name, namelen); -#else - /* this OS doesn't have the Linux abstract namespace */ - - namelen = strlen(name) + strlen(FILESYSTEM_SOCKET_PREFIX); - /* unix_path_max appears to be missing on linux */ - if (namelen > sizeof(*p_addr) - offsetof(struct sockaddr_un, sun_path) - 1) { - goto error; - } - - strcpy(p_addr->sun_path, FILESYSTEM_SOCKET_PREFIX); - strcat(p_addr->sun_path, name); -#endif - break; - - case ANDROID_SOCKET_NAMESPACE_RESERVED: - namelen = strlen(name) + strlen(ANDROID_RESERVED_SOCKET_PREFIX); - /* unix_path_max appears to be missing on linux */ - if (namelen > sizeof(*p_addr) - offsetof(struct sockaddr_un, sun_path) - 1) { - goto error; - } - - strcpy(p_addr->sun_path, ANDROID_RESERVED_SOCKET_PREFIX); - strcat(p_addr->sun_path, name); - break; - - case ANDROID_SOCKET_NAMESPACE_FILESYSTEM: - namelen = strlen(name); - /* unix_path_max appears to be missing on linux */ - if (namelen > sizeof(*p_addr) - offsetof(struct sockaddr_un, sun_path) - 1) { - goto error; - } - - strcpy(p_addr->sun_path, name); - break; - - default: - /* invalid namespace id */ - return -1; + std::string path = "/dev/socket/" + name; + if (path.size() + 1 > sizeof(addr.sun_path)) { + return -1; } + strlcpy(addr.sun_path, path.c_str(), sizeof(addr.sun_path)); - p_addr->sun_family = AF_LOCAL; - *alen = namelen + offsetof(struct sockaddr_un, sun_path) + 1; - return 0; -error: - return -1; -} - -/** - * connect to peer named "name" on fd - * returns same fd or -1 on error. - * fd is not closed on error. that's your job. - * - * Used by AndroidSocketImpl - */ -LIBLOG_WEAK int socket_local_client_connect(int fd, const char* name, int namespaceId, - int type __unused) { - struct sockaddr_un addr; - socklen_t alen; - int err; - - err = socket_make_sockaddr_un(name, namespaceId, &addr, &alen); - - if (err < 0) { - goto error; - } - - if (connect(fd, (struct sockaddr*)&addr, alen) < 0) { - goto error; - } - - return fd; - -error: - return -1; -} - -/** - * connect to peer named "name" - * returns fd or -1 on error - */ -LIBLOG_WEAK int socket_local_client(const char* name, int namespaceId, int type) { - int s; - - s = socket(AF_LOCAL, type, 0); - if (s < 0) return -1; - - if (0 > socket_local_client_connect(s, name, namespaceId, type)) { - close(s); + int fd = socket(AF_LOCAL, type | SOCK_CLOEXEC, 0); + if (fd == 0) { return -1; } - return s; -} + if (connect(fd, reinterpret_cast(&addr), sizeof(addr)) == -1) { + close(fd); + return -1; + } -#endif /* !_WIN32 */ -/* End of ../libcutils/socket_local_client.c */ + return fd; +} /* worker for sending the command to the logger */ static ssize_t send_log_msg(struct android_log_logger* logger, const char* msg, char* buf, @@ -250,7 +131,7 @@ static ssize_t send_log_msg(struct android_log_logger* logger, const char* msg, size_t len; char* cp; int errno_save = 0; - int sock = socket_local_client("logd", ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_STREAM); + int sock = socket_local_client("logd", SOCK_STREAM); if (sock < 0) { return sock; } @@ -460,10 +341,10 @@ static int logdOpen(struct android_log_logger_list* logger_list, return sock; } - sock = socket_local_client("logdr", ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET); + sock = socket_local_client("logdr", SOCK_SEQPACKET); if (sock == 0) { /* Guarantee not file descriptor zero */ - int newsock = socket_local_client("logdr", ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET); + int newsock = socket_local_client("logdr", SOCK_SEQPACKET); close(sock); sock = newsock; } diff --git a/liblog/tests/Android.bp b/liblog/tests/Android.bp index 4ab2acb2c..50755ce82 100644 --- a/liblog/tests/Android.bp +++ b/liblog/tests/Android.bp @@ -31,6 +31,7 @@ cc_benchmark { shared_libs: [ "libm", "libbase", + "libcutils", ], static_libs: ["liblog"], srcs: ["liblog_benchmark.cpp"], diff --git a/liblog/tests/libc_test.cpp b/liblog/tests/libc_test.cpp index 6d78ed6db..3534eb8d3 100644 --- a/liblog/tests/libc_test.cpp +++ b/liblog/tests/libc_test.cpp @@ -23,7 +23,7 @@ TEST(libc, __pstore_append) { #ifdef __ANDROID__ #ifndef NO_PSTORE FILE* fp; - ASSERT_TRUE(NULL != (fp = fopen("/dev/pmsg0", "a"))); + ASSERT_TRUE(NULL != (fp = fopen("/dev/pmsg0", "ae"))); static const char message[] = "libc.__pstore_append\n"; ASSERT_EQ((size_t)1, fwrite(message, sizeof(message), 1, fp)); int fflushReturn = fflush(fp); diff --git a/liblog/tests/liblog_benchmark.cpp b/liblog/tests/liblog_benchmark.cpp index c8ac321f8..7d11ccf71 100644 --- a/liblog/tests/liblog_benchmark.cpp +++ b/liblog/tests/liblog_benchmark.cpp @@ -172,7 +172,7 @@ BENCHMARK(BM_time_time); * Measure the time it takes to submit the android logging data to pstore */ static void BM_pmsg_short(benchmark::State& state) { - int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY)); + int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY | O_CLOEXEC)); if (pstore_fd < 0) { state.SkipWithError("/dev/pmsg0"); return; @@ -248,7 +248,7 @@ BENCHMARK(BM_pmsg_short); * best case aligned single block. */ static void BM_pmsg_short_aligned(benchmark::State& state) { - int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY)); + int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY | O_CLOEXEC)); if (pstore_fd < 0) { state.SkipWithError("/dev/pmsg0"); return; @@ -323,7 +323,7 @@ BENCHMARK(BM_pmsg_short_aligned); * best case aligned single block. */ static void BM_pmsg_short_unaligned1(benchmark::State& state) { - int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY)); + int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY | O_CLOEXEC)); if (pstore_fd < 0) { state.SkipWithError("/dev/pmsg0"); return; @@ -398,7 +398,7 @@ BENCHMARK(BM_pmsg_short_unaligned1); * best case aligned single block. */ static void BM_pmsg_long_aligned(benchmark::State& state) { - int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY)); + int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY | O_CLOEXEC)); if (pstore_fd < 0) { state.SkipWithError("/dev/pmsg0"); return; @@ -471,7 +471,7 @@ BENCHMARK(BM_pmsg_long_aligned); * best case aligned single block. */ static void BM_pmsg_long_unaligned1(benchmark::State& state) { - int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY)); + int pstore_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY | O_CLOEXEC)); if (pstore_fd < 0) { state.SkipWithError("/dev/pmsg0"); return; @@ -949,8 +949,8 @@ BENCHMARK(BM_lookupEventTagNum); // Must be functionally identical to liblog internal __send_log_msg. static void send_to_control(char* buf, size_t len) { - int sock = socket_local_client("logd", ANDROID_SOCKET_NAMESPACE_RESERVED, - SOCK_STREAM); + int sock = + socket_local_client("logd", ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_STREAM | SOCK_CLOEXEC); if (sock < 0) return; size_t writeLen = strlen(buf) + 1; diff --git a/liblog/tests/liblog_test.cpp b/liblog/tests/liblog_test.cpp index 83f0fa91f..1f87b3eae 100644 --- a/liblog/tests/liblog_test.cpp +++ b/liblog/tests/liblog_test.cpp @@ -93,7 +93,7 @@ TEST(liblog, __android_log_btwrite) { static std::string popenToString(const std::string& command) { std::string ret; - FILE* fp = popen(command.c_str(), "r"); + FILE* fp = popen(command.c_str(), "re"); if (fp) { if (!android::base::ReadFdToString(fileno(fp), &ret)) ret = ""; pclose(fp); @@ -645,7 +645,7 @@ static void get_ticks(unsigned long long* uticks, unsigned long long* sticks) { char buffer[512]; snprintf(buffer, sizeof(buffer), "/proc/%u/stat", pid); - FILE* fp = fopen(buffer, "r"); + FILE* fp = fopen(buffer, "re"); if (!fp) { return; } diff --git a/liblog/tests/log_radio_test.cpp b/liblog/tests/log_radio_test.cpp index f202c677f..fa1255e0c 100644 --- a/liblog/tests/log_radio_test.cpp +++ b/liblog/tests/log_radio_test.cpp @@ -91,7 +91,7 @@ TEST(liblog, RLOG) { "logcat -b radio --pid=%u -d -s" " TEST__RLOGV TEST__RLOGD TEST__RLOGI TEST__RLOGW TEST__RLOGE", (unsigned)getpid()); - FILE* fp = popen(buf.c_str(), "r"); + FILE* fp = popen(buf.c_str(), "re"); int count = 0; int count_false = 0; if (fp) { diff --git a/liblog/tests/log_system_test.cpp b/liblog/tests/log_system_test.cpp index 0656c0b23..13f026d34 100644 --- a/liblog/tests/log_system_test.cpp +++ b/liblog/tests/log_system_test.cpp @@ -91,7 +91,7 @@ TEST(liblog, SLOG) { "logcat -b system --pid=%u -d -s" " TEST__SLOGV TEST__SLOGD TEST__SLOGI TEST__SLOGW TEST__SLOGE", (unsigned)getpid()); - FILE* fp = popen(buf.c_str(), "r"); + FILE* fp = popen(buf.c_str(), "re"); int count = 0; int count_false = 0; if (fp) {