liblog: remove Rwlocks for logd_socket and pmsg_fd
These historically used atomics to manage their lifetime. They were
unfortunately unsafe and later replace with a RwLock. A lock is
also problematic as it is too heavy weight for the typical use case
and implies that logging is neither async nor fork safe.
This change returns us to using atomics with two key changes:
1) compare_exchange_strong() is used instead of atomic_exchange().
The latter has a race condition where a separate thread could have
read the atomic value into a register, while the thread performing
the atomic_exchange closes that FD. The new code only changes the
FD in the atomic if it is uninitialized.
2) Using the fact that DGRAM sockets can have connect() called on them
multiple times, it uses a single logd_socket for the duration of
the program.
These sockets are thread/async/fork safely created and accessed.
The one caveat is __android_log_close(), which is intended only to be
used by zygote when it is single threaded and is therefore not thread
safe. It will close this socket and reset the underlying variable,
such that the next log message will go through the above
initialization.
Bug: 65062446
Test: logging works, logging unit tests
Test: new unit test
Change-Id: Ia4dbf7479dbe50683d124558ab2f83bff53b8f5f
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>();
-}