Merge "liblog: remove Rwlocks for logd_socket and pmsg_fd" into rvc-dev
diff --git a/liblog/include/log/log.h b/liblog/include/log/log.h
index 90d1e76..c116add 100644
--- a/liblog/include/log/log.h
+++ b/liblog/include/log/log.h
@@ -143,8 +143,11 @@
/*
* Release any logger resources (a new log write will immediately re-acquire)
*
- * May be used to clean up File descriptors after a Fork, the resources are
- * all O_CLOEXEC so wil self clean on exec().
+ * This is specifically meant to be used by Zygote to close open file descriptors after fork()
+ * and before specialization. O_CLOEXEC is used on file descriptors, so they will be closed upon
+ * exec() in normal use cases.
+ *
+ * Note that this is not safe to call from a multi-threaded program.
*/
void __android_log_close(void);
diff --git a/liblog/logd_writer.cpp b/liblog/logd_writer.cpp
index 67376f4..a230749 100644
--- a/liblog/logd_writer.cpp
+++ b/liblog/logd_writer.cpp
@@ -32,58 +32,53 @@
#include <time.h>
#include <unistd.h>
-#include <shared_mutex>
-
#include <private/android_filesystem_config.h>
#include <private/android_logger.h>
#include "logger.h"
-#include "rwlock.h"
#include "uio.h"
-static int logd_socket;
-static RwLock logd_socket_lock;
+static atomic_int logd_socket;
-static void OpenSocketLocked() {
- logd_socket = TEMP_FAILURE_RETRY(socket(PF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0));
- if (logd_socket <= 0) {
- return;
- }
-
+// Note that it is safe to call connect() multiple times on DGRAM Unix domain sockets, so this
+// function is used to reconnect to logd without requiring a new socket.
+static void LogdConnect() {
sockaddr_un un = {};
un.sun_family = AF_UNIX;
strcpy(un.sun_path, "/dev/socket/logdw");
-
- if (TEMP_FAILURE_RETRY(
- connect(logd_socket, reinterpret_cast<sockaddr*>(&un), sizeof(sockaddr_un))) < 0) {
- close(logd_socket);
- logd_socket = 0;
- }
+ TEMP_FAILURE_RETRY(connect(logd_socket, reinterpret_cast<sockaddr*>(&un), sizeof(sockaddr_un)));
}
-static void OpenSocket() {
- auto lock = std::unique_lock{logd_socket_lock};
- if (logd_socket > 0) {
- // Someone raced us and opened the socket already.
+// logd_socket should only be opened once. If we see that logd_socket is uninitialized, we create a
+// new socket and attempt to exchange it into the atomic logd_socket. If the compare/exchange was
+// successful, then that will be the socket used for the duration of the program, otherwise a
+// different thread has already opened and written the socket to the atomic, so close the new socket
+// and return.
+static void GetSocket() {
+ if (logd_socket != 0) {
return;
}
- OpenSocketLocked();
-}
-
-static void ResetSocket(int old_socket) {
- auto lock = std::unique_lock{logd_socket_lock};
- if (old_socket != logd_socket) {
- // Someone raced us and reset the socket already.
+ int new_socket =
+ TEMP_FAILURE_RETRY(socket(PF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0));
+ if (new_socket <= 0) {
return;
}
- close(logd_socket);
- logd_socket = 0;
- OpenSocketLocked();
+
+ int uninitialized_value = 0;
+ if (!logd_socket.compare_exchange_strong(uninitialized_value, new_socket)) {
+ close(new_socket);
+ return;
+ }
+
+ LogdConnect();
}
+// This is the one exception to the above. Zygote uses this to clean up open FD's after fork() and
+// before specialization. It is single threaded at this point and therefore this function is
+// explicitly not thread safe. It sets logd_socket to 0, so future logs will be safely initialized
+// whenever they happen.
void LogdClose() {
- auto lock = std::unique_lock{logd_socket_lock};
if (logd_socket > 0) {
close(logd_socket);
}
@@ -99,12 +94,7 @@
static atomic_int dropped;
static atomic_int droppedSecurity;
- auto lock = std::shared_lock{logd_socket_lock};
- if (logd_socket <= 0) {
- lock.unlock();
- OpenSocket();
- lock.lock();
- }
+ GetSocket();
if (logd_socket <= 0) {
return -EBADF;
@@ -183,10 +173,7 @@
// the connection, so we reset it and try again.
ret = TEMP_FAILURE_RETRY(writev(logd_socket, newVec, i));
if (ret < 0 && errno != EAGAIN) {
- int old_socket = logd_socket;
- lock.unlock();
- ResetSocket(old_socket);
- lock.lock();
+ LogdConnect();
ret = TEMP_FAILURE_RETRY(writev(logd_socket, newVec, i));
}
diff --git a/liblog/pmsg_writer.cpp b/liblog/pmsg_writer.cpp
index 06e5e04..0751e2c 100644
--- a/liblog/pmsg_writer.cpp
+++ b/liblog/pmsg_writer.cpp
@@ -23,30 +23,36 @@
#include <sys/types.h>
#include <time.h>
-#include <shared_mutex>
-
#include <log/log_properties.h>
#include <private/android_logger.h>
#include "logger.h"
-#include "rwlock.h"
#include "uio.h"
-static int pmsg_fd;
-static RwLock pmsg_fd_lock;
+static atomic_int pmsg_fd;
-static void PmsgOpen() {
- auto lock = std::unique_lock{pmsg_fd_lock};
- if (pmsg_fd > 0) {
- // Someone raced us and opened the socket already.
+// pmsg_fd should only beopened once. If we see that pmsg_fd is uninitialized, we open "/dev/pmsg0"
+// then attempt to compare/exchange it into pmsg_fd. If the compare/exchange was successful, then
+// that will be the fd used for the duration of the program, otherwise a different thread has
+// already opened and written the fd to the atomic, so close the new fd and return.
+static void GetPmsgFd() {
+ if (pmsg_fd != 0) {
return;
}
- pmsg_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY | O_CLOEXEC));
+ int new_fd = TEMP_FAILURE_RETRY(open("/dev/pmsg0", O_WRONLY | O_CLOEXEC));
+ if (new_fd <= 0) {
+ return;
+ }
+
+ int uninitialized_value = 0;
+ if (!pmsg_fd.compare_exchange_strong(uninitialized_value, new_fd)) {
+ close(new_fd);
+ return;
+ }
}
void PmsgClose() {
- auto lock = std::unique_lock{pmsg_fd_lock};
if (pmsg_fd > 0) {
close(pmsg_fd);
}
@@ -77,13 +83,7 @@
}
}
- auto lock = std::shared_lock{pmsg_fd_lock};
-
- if (pmsg_fd <= 0) {
- lock.unlock();
- PmsgOpen();
- lock.lock();
- }
+ GetPmsgFd();
if (pmsg_fd <= 0) {
return -EBADF;
diff --git a/liblog/rwlock.h b/liblog/rwlock.h
deleted file mode 100644
index 00f1806..0000000
--- a/liblog/rwlock.h
+++ /dev/null
@@ -1,39 +0,0 @@
-/*
- * Copyright (C) 2019 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.
- */
-
-#pragma once
-
-#include <pthread.h>
-
-// As of the end of Dec 2019, std::shared_mutex is *not* simply a pthread_rwlock, but rather a
-// combination of std::mutex and std::condition variable, which is obviously less efficient. This
-// immitates what std::shared_mutex should be doing and is compatible with std::shared_lock and
-// std::unique_lock.
-
-class RwLock {
- public:
- RwLock() {}
- ~RwLock() {}
-
- void lock() { pthread_rwlock_wrlock(&rwlock_); }
- void unlock() { pthread_rwlock_unlock(&rwlock_); }
-
- void lock_shared() { pthread_rwlock_rdlock(&rwlock_); }
- void unlock_shared() { pthread_rwlock_unlock(&rwlock_); }
-
- private:
- pthread_rwlock_t rwlock_ = PTHREAD_RWLOCK_INITIALIZER;
-};
diff --git a/liblog/tests/Android.bp b/liblog/tests/Android.bp
index 385b079..50800c5 100644
--- a/liblog/tests/Android.bp
+++ b/liblog/tests/Android.bp
@@ -63,8 +63,8 @@
"log_system_test.cpp",
"log_time_test.cpp",
"log_wrap_test.cpp",
+ "logd_writer_test.cpp",
"logprint_test.cpp",
- "rwlock_test.cpp",
],
shared_libs: [
"libcutils",
@@ -108,7 +108,6 @@
"liblog_host_test.cpp",
"liblog_default_tag.cpp",
"liblog_global_state.cpp",
- "rwlock_test.cpp",
],
isolated: true,
}
diff --git a/liblog/tests/logd_writer_test.cpp b/liblog/tests/logd_writer_test.cpp
new file mode 100644
index 0000000..c856720
--- /dev/null
+++ b/liblog/tests/logd_writer_test.cpp
@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) 2020 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.
+ */
+
+#include <sys/un.h>
+#include <unistd.h>
+
+#include <android-base/file.h>
+#include <android-base/stringprintf.h>
+#include <android-base/unique_fd.h>
+#include <gtest/gtest.h>
+
+using android::base::StringPrintf;
+using android::base::unique_fd;
+
+// logd_writer takes advantage of the fact that connect() can be called multiple times for a DGRAM
+// socket. This tests for that behavior.
+TEST(liblog, multi_connect_dgram_socket) {
+#ifdef __ANDROID__
+ auto temp_dir = TemporaryDir();
+ auto socket_path = StringPrintf("%s/test_socket", temp_dir.path);
+
+ unique_fd server_socket;
+
+ auto open_server_socket = [&] {
+ server_socket.reset(TEMP_FAILURE_RETRY(socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0)));
+ ASSERT_TRUE(server_socket.ok());
+
+ sockaddr_un server_sockaddr = {};
+ server_sockaddr.sun_family = AF_UNIX;
+ strlcpy(server_sockaddr.sun_path, socket_path.c_str(), sizeof(server_sockaddr.sun_path));
+ ASSERT_EQ(0,
+ TEMP_FAILURE_RETRY(bind(server_socket, reinterpret_cast<sockaddr*>(&server_sockaddr),
+ sizeof(server_sockaddr))));
+ };
+
+ // Open the server socket.
+ open_server_socket();
+
+ // Open the client socket.
+ auto client_socket =
+ unique_fd{TEMP_FAILURE_RETRY(socket(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0))};
+ ASSERT_TRUE(client_socket.ok());
+ sockaddr_un client_sockaddr = {};
+ client_sockaddr.sun_family = AF_UNIX;
+ strlcpy(client_sockaddr.sun_path, socket_path.c_str(), sizeof(client_sockaddr.sun_path));
+ ASSERT_EQ(0,
+ TEMP_FAILURE_RETRY(connect(client_socket, reinterpret_cast<sockaddr*>(&client_sockaddr),
+ sizeof(client_sockaddr))));
+
+ // Ensure that communication works.
+ constexpr static char kSmoke[] = "smoke test";
+ ssize_t smoke_len = sizeof(kSmoke);
+ ASSERT_EQ(smoke_len, TEMP_FAILURE_RETRY(write(client_socket, kSmoke, sizeof(kSmoke))));
+ char read_buf[512];
+ ASSERT_EQ(smoke_len, TEMP_FAILURE_RETRY(read(server_socket, read_buf, sizeof(read_buf))));
+ ASSERT_STREQ(kSmoke, read_buf);
+
+ // Close the server socket.
+ server_socket.reset();
+ ASSERT_EQ(0, unlink(socket_path.c_str())) << strerror(errno);
+
+ // Ensure that write() from the client returns an error since the server is closed.
+ ASSERT_EQ(-1, TEMP_FAILURE_RETRY(write(client_socket, kSmoke, sizeof(kSmoke))));
+ ASSERT_EQ(errno, ECONNREFUSED) << strerror(errno);
+
+ // Open the server socket again.
+ open_server_socket();
+
+ // Reconnect the same client socket.
+ ASSERT_EQ(0,
+ TEMP_FAILURE_RETRY(connect(client_socket, reinterpret_cast<sockaddr*>(&client_sockaddr),
+ sizeof(client_sockaddr))))
+ << strerror(errno);
+
+ // Ensure that communication works.
+ ASSERT_EQ(smoke_len, TEMP_FAILURE_RETRY(write(client_socket, kSmoke, sizeof(kSmoke))));
+ ASSERT_EQ(smoke_len, TEMP_FAILURE_RETRY(read(server_socket, read_buf, sizeof(read_buf))));
+ ASSERT_STREQ(kSmoke, read_buf);
+#else
+ GTEST_LOG_(INFO) << "This test does nothing.\n";
+#endif
+}
\ No newline at end of file
diff --git a/liblog/tests/rwlock_test.cpp b/liblog/tests/rwlock_test.cpp
deleted file mode 100644
index 617d5c4..0000000
--- a/liblog/tests/rwlock_test.cpp
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * Copyright (C) 2019 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.
- */
-
-#include "../rwlock.h"
-
-#include <chrono>
-#include <shared_mutex>
-#include <thread>
-
-#include <gtest/gtest.h>
-
-using namespace std::literals;
-
-TEST(rwlock, reader_then_reader_lock) {
- RwLock lock;
-
- bool thread_ran = false;
- auto read_guard = std::shared_lock{lock};
-
- auto reader_thread = std::thread([&] {
- auto read_guard = std::shared_lock{lock};
- thread_ran = true;
- });
-
- auto end_time = std::chrono::steady_clock::now() + 1s;
-
- while (std::chrono::steady_clock::now() < end_time) {
- if (thread_ran) {
- break;
- }
- }
-
- EXPECT_EQ(true, thread_ran);
-
- // Unlock the lock in case something went wrong, to ensure that we can still join() the thread.
- read_guard.unlock();
- reader_thread.join();
-}
-
-template <template <typename> typename L1, template <typename> typename L2>
-void TestBlockingLocks() {
- RwLock lock;
-
- bool thread_ran = false;
- auto read_guard = L1{lock};
-
- auto reader_thread = std::thread([&] {
- auto read_guard = L2{lock};
- thread_ran = true;
- });
-
- auto end_time = std::chrono::steady_clock::now() + 1s;
-
- while (std::chrono::steady_clock::now() < end_time) {
- if (thread_ran) {
- break;
- }
- }
-
- EXPECT_EQ(false, thread_ran);
-
- read_guard.unlock();
- reader_thread.join();
-
- EXPECT_EQ(true, thread_ran);
-}
-
-TEST(rwlock, reader_then_writer_lock) {
- TestBlockingLocks<std::shared_lock, std::unique_lock>();
-}
-
-TEST(rwlock, writer_then_reader_lock) {
- TestBlockingLocks<std::unique_lock, std::shared_lock>();
-}
-
-TEST(rwlock, writer_then_writer_lock) {
- TestBlockingLocks<std::unique_lock, std::unique_lock>();
-}