From 5f77d17b69c405859f73dabd3500f8e3e6286b74 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 2 Dec 2015 17:30:58 -0800 Subject: [PATCH 01/19] adbd: split up writes longer than 16k. Also, inline the bulk_read and bulk_write functions which were only being used by one other function. Bug: http://b/25847115 Change-Id: I218a869030219f606577a5529601c542488115e0 --- adb/usb_linux_client.cpp | 60 +++++++++++++++------------------------- 1 file changed, 22 insertions(+), 38 deletions(-) diff --git a/adb/usb_linux_client.cpp b/adb/usb_linux_client.cpp index 3495a71cc..ceed8facf 100644 --- a/adb/usb_linux_client.cpp +++ b/adb/usb_linux_client.cpp @@ -456,56 +456,40 @@ static void *usb_ffs_open_thread(void *x) return 0; } -static int bulk_write(int bulk_in, const uint8_t* buf, size_t length) -{ - size_t count = 0; +static int usb_ffs_write(usb_handle* h, const void* data, int len) { + D("about to write (fd=%d, len=%d)", h->bulk_in, len); - while (count < length) { - int ret = adb_write(bulk_in, buf + count, length - count); - if (ret < 0) return -1; - count += ret; + // Writes larger than 16k fail on some devices (seed with 3.10.49-g209ea2f in particular). + const char* buf = static_cast(data); + while (len > 0) { + int write_len = (len > 16384) ? 16384 : len; + int n = adb_write(h->bulk_in, buf, write_len); + if (n < 0) { + D("ERROR: fd = %d, n = %d: %s", h->bulk_in, n, strerror(errno)); + return -1; + } + buf += n; + len -= n; } - D("[ bulk_write done fd=%d ]", bulk_in); - return count; -} - -static int usb_ffs_write(usb_handle* h, const void* data, int len) -{ - D("about to write (fd=%d, len=%d)", h->bulk_in, len); - int n = bulk_write(h->bulk_in, reinterpret_cast(data), len); - if (n != len) { - D("ERROR: fd = %d, n = %d: %s", h->bulk_in, n, strerror(errno)); - return -1; - } D("[ done fd=%d ]", h->bulk_in); return 0; } -static int bulk_read(int bulk_out, uint8_t* buf, size_t length) -{ - size_t count = 0; +static int usb_ffs_read(usb_handle* h, void* data, int len) { + D("about to read (fd=%d, len=%d)", h->bulk_out, len); - while (count < length) { - int ret = adb_read(bulk_out, buf + count, length - count); - if (ret < 0) { - D("[ bulk_read failed fd=%d length=%zu count=%zu ]", bulk_out, length, count); + char* buf = static_cast(data); + while (len > 0) { + int n = adb_read(h->bulk_out, buf, len); + if (n < 0) { + D("ERROR: fd = %d, n = %d: %s", h->bulk_out, n, strerror(errno)); return -1; } - count += ret; + buf += n; + len -= n; } - return count; -} - -static int usb_ffs_read(usb_handle* h, void* data, int len) -{ - D("about to read (fd=%d, len=%d)", h->bulk_out, len); - int n = bulk_read(h->bulk_out, reinterpret_cast(data), len); - if (n != len) { - D("ERROR: fd = %d, n = %d: %s", h->bulk_out, n, strerror(errno)); - return -1; - } D("[ done fd=%d ]", h->bulk_out); return 0; } From 6e55603e937b6eb76b69832ae2c71dd14cee8963 Mon Sep 17 00:00:00 2001 From: Dimitry Ivanov Date: Wed, 13 Jan 2016 15:19:35 -0800 Subject: [PATCH 02/19] Add libRS.so to the list of public libraries Bug: http://b/26509995 Change-Id: I8c0ae3629928171d229a901cfc997780665e3ae8 --- libnativeloader/native_loader.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libnativeloader/native_loader.cpp b/libnativeloader/native_loader.cpp index 29c5a0ffc..2e2ef366f 100644 --- a/libnativeloader/native_loader.cpp +++ b/libnativeloader/native_loader.cpp @@ -52,6 +52,7 @@ static const char* kPublicNativeLibraries = "libandroid.so:" "libm.so:" "libOpenMAXAL.so:" "libOpenSLES.so:" + "libRS.so:" "libstdc++.so:" "libwebviewchromium_plat_support.so:" "libz.so"; From 43ea1c58b857d5610efb352fd81db33488fdabf6 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 14 Jan 2016 11:53:42 -0700 Subject: [PATCH 03/19] Be strict, but not that strict. Certain apps decide that they want to chmod() their private data directories to gain more security. We still want to carefully enforce owner UID/GID, but relax the mode check for now. Bug: 26549892 Change-Id: I362d530ba0b20fb23f427ac082ee003864adc57d --- include/cutils/fs.h | 2 +- libcutils/fs.c | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/include/cutils/fs.h b/include/cutils/fs.h index 98c12962a..be9e819dc 100644 --- a/include/cutils/fs.h +++ b/include/cutils/fs.h @@ -47,7 +47,7 @@ extern int fs_prepare_dir(const char* path, mode_t mode, uid_t uid, gid_t gid); /* * Ensure that directory exists with given mode and owners. If it exists - * with a different mode or owners, they are not fixed and -1 is returned. + * with different owners, they are not fixed and -1 is returned. */ extern int fs_prepare_dir_strict(const char* path, mode_t mode, uid_t uid, gid_t gid); diff --git a/libcutils/fs.c b/libcutils/fs.c index 88c488cba..5e2ef0b8d 100644 --- a/libcutils/fs.c +++ b/libcutils/fs.c @@ -55,13 +55,22 @@ static int fs_prepare_dir_impl(const char* path, mode_t mode, uid_t uid, gid_t g ALOGE("Not a directory: %s", path); return -1; } - if (((sb.st_mode & ALL_PERMS) == mode) && (sb.st_uid == uid) && (sb.st_gid == gid)) { + int owner_match = ((sb.st_uid == uid) && (sb.st_gid == gid)); + int mode_match = ((sb.st_mode & ALL_PERMS) == mode); + if (owner_match && mode_match) { return 0; } else if (allow_fixup) { goto fixup; } else { - ALOGE("Path %s exists with unexpected permissions", path); - return -1; + if (!owner_match) { + ALOGE("Expected path %s with owner %d:%d but found %d:%d", + path, uid, gid, sb.st_uid, sb.st_gid); + return -1; + } else { + ALOGW("Expected path %s with mode %o but found %o", + path, mode, (sb.st_mode & ALL_PERMS)); + return 0; + } } create: From 8f860fd3327ab5f9a6f277e2fd5fa4dbf8fca6c1 Mon Sep 17 00:00:00 2001 From: Calin Juravle Date: Wed, 27 Jan 2016 17:52:35 +0000 Subject: [PATCH 04/19] Revert "nativebrige: log code_cache access errors to stderr as well" This reverts commit 6d5017803e5fec20fb0e1ee178412748eb2b7279. Bug: 26675310 Change-Id: Id56b5e832c85f01fca0a2198499d24224c5a6878 --- libnativebridge/native_bridge.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/libnativebridge/native_bridge.cc b/libnativebridge/native_bridge.cc index 4eff89115..32a65ea04 100644 --- a/libnativebridge/native_bridge.cc +++ b/libnativebridge/native_bridge.cc @@ -413,19 +413,14 @@ bool InitializeNativeBridge(JNIEnv* env, const char* instruction_set) { if (errno == ENOENT) { if (mkdir(app_code_cache_dir, S_IRWXU | S_IRWXG | S_IXOTH) == -1) { ALOGW("Cannot create code cache directory %s: %s.", app_code_cache_dir, strerror(errno)); - fprintf(stderr, "Cannot create code cache directory %s: %s.", - app_code_cache_dir, strerror(errno)); ReleaseAppCodeCacheDir(); } } else { ALOGW("Cannot stat code cache directory %s: %s.", app_code_cache_dir, strerror(errno)); - fprintf(stderr, "Cannot stat code cache directory %s: %s.", - app_code_cache_dir, strerror(errno)); ReleaseAppCodeCacheDir(); } } else if (!S_ISDIR(st.st_mode)) { ALOGW("Code cache is not a directory %s.", app_code_cache_dir); - fprintf(stderr, "Code cache is not a directory %s.", app_code_cache_dir); ReleaseAppCodeCacheDir(); } From 49708de906dc19090b277b75785a1fbcb121edff Mon Sep 17 00:00:00 2001 From: Dimitry Ivanov Date: Wed, 10 Feb 2016 14:09:22 -0800 Subject: [PATCH 05/19] Preload public native libraries Preload libraries needed by the public namespace at the earlier stage. This saves time on InitPublicNamespace and saves memory because the libraries are linked before zygote fork. Bug: http://b/26409579 Change-Id: I59153a4180b930f31b542d8d2cb17b5d63c36774 --- libnativeloader/native_loader.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/libnativeloader/native_loader.cpp b/libnativeloader/native_loader.cpp index 75d2a2db7..aaff64c35 100644 --- a/libnativeloader/native_loader.cpp +++ b/libnativeloader/native_loader.cpp @@ -62,7 +62,9 @@ static const char* kPublicNativeLibraries = "libandroid.so:" class LibraryNamespaces { public: - LibraryNamespaces() : initialized_(false) { } + LibraryNamespaces() : initialized_(false) { + PreloadPublicLibraries(); + } android_namespace_t* GetOrCreate(JNIEnv* env, jobject class_loader, bool is_shared, @@ -108,15 +110,16 @@ class LibraryNamespaces { } private: - bool InitPublicNamespace(const char* library_path) { - // Make sure all the public libraries are loaded + void PreloadPublicLibraries() { + // android_init_namespaces() expects all the public libraries + // to be loaded so that they can be found by soname alone. std::vector sonames = android::base::Split(kPublicNativeLibraries, ":"); for (const auto& soname : sonames) { - if (dlopen(soname.c_str(), RTLD_NOW | RTLD_NODELETE) == nullptr) { - return false; - } + dlopen(soname.c_str(), RTLD_NOW | RTLD_NODELETE); } + } + bool InitPublicNamespace(const char* library_path) { // Some apps call dlopen from generated code unknown to linker in which // case linker uses anonymous namespace. See b/25844435 for details. initialized_ = android_init_namespaces(kPublicNativeLibraries, library_path); From 12f74e63a512ddb756c0d434ef15addad6931f4e Mon Sep 17 00:00:00 2001 From: Badhri Jagan Sridharan Date: Tue, 27 Oct 2015 10:43:53 -0700 Subject: [PATCH 06/19] healthd: Read the max power supply voltage With Type-C PD, VBUS can no longer be assumed to to be at 5V. Read the "voltage_max" field from the power_supply class node and export it through BatteryProperties service. Bug: 25229483 Change-Id: I04e32d9783a21bab375f1724758a9eeace2a047c --- healthd/BatteryMonitor.cpp | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/healthd/BatteryMonitor.cpp b/healthd/BatteryMonitor.cpp index 48af70ea7..110ed038f 100644 --- a/healthd/BatteryMonitor.cpp +++ b/healthd/BatteryMonitor.cpp @@ -39,6 +39,8 @@ #define FAKE_BATTERY_CAPACITY 42 #define FAKE_BATTERY_TEMPERATURE 424 #define ALWAYS_PLUGGED_CAPACITY 100 +#define MILLION 10000000.0 +#define DEFAULT_VBUS_VOLTAGE 5000000 namespace android { @@ -61,6 +63,7 @@ static void initBatteryProperties(BatteryProperties* props) { props->chargerUsbOnline = false; props->chargerWirelessOnline = false; props->maxChargingCurrent = 0; + props->maxChargingVoltage = 0; props->batteryStatus = BATTERY_STATUS_UNKNOWN; props->batteryHealth = BATTERY_HEALTH_UNKNOWN; props->batteryPresent = false; @@ -254,6 +257,7 @@ bool BatteryMonitor::update(void) { props.batteryTechnology = String8(buf); unsigned int i; + double MaxPower = 0; for (i = 0; i < mChargerNames.size(); i++) { String8 path; @@ -282,11 +286,23 @@ bool BatteryMonitor::update(void) { path.clear(); path.appendFormat("%s/%s/current_max", POWER_SUPPLY_SYSFS_PATH, mChargerNames[i].string()); - if (access(path.string(), R_OK) == 0) { - int maxChargingCurrent = getIntField(path); - if (props.maxChargingCurrent < maxChargingCurrent) { - props.maxChargingCurrent = maxChargingCurrent; - } + int ChargingCurrent = + (access(path.string(), R_OK) == 0) ? getIntField(path) : 0; + + path.clear(); + path.appendFormat("%s/%s/voltage_max", POWER_SUPPLY_SYSFS_PATH, + mChargerNames[i].string()); + + int ChargingVoltage = + (access(path.string(), R_OK) == 0) ? getIntField(path) : + DEFAULT_VBUS_VOLTAGE; + + double power = ((double)ChargingCurrent / MILLION) * + ((double)ChargingVoltage / MILLION); + if (MaxPower < power) { + props.maxChargingCurrent = ChargingCurrent; + props.maxChargingVoltage = ChargingVoltage; + MaxPower = power; } } } @@ -416,9 +432,10 @@ void BatteryMonitor::dumpState(int fd) { int v; char vs[128]; - snprintf(vs, sizeof(vs), "ac: %d usb: %d wireless: %d current_max: %d\n", + snprintf(vs, sizeof(vs), "ac: %d usb: %d wireless: %d current_max: %d voltage_max: %d\n", props.chargerAcOnline, props.chargerUsbOnline, - props.chargerWirelessOnline, props.maxChargingCurrent); + props.chargerWirelessOnline, props.maxChargingCurrent, + props.maxChargingVoltage); write(fd, vs, strlen(vs)); snprintf(vs, sizeof(vs), "status: %d health: %d present: %d\n", props.batteryStatus, props.batteryHealth, props.batteryPresent); From fd8dd0b6cb1d7915bc20f459a3f82d134888bb21 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Tue, 23 Feb 2016 18:02:20 -0800 Subject: [PATCH 07/19] Fix incorrect handling of snprintf return value. The code assumed that snprintf would never return a value less than the passed in size of the buffer. This is not accurate, so fix all of the places this assumptions is made. Also, if the name is too large, then truncate just the name to make everything fit. Added a new set of tests for this code. Verified that the old code passes on the _normal and _exact version of the tests, but fails with the FORTIFY error on the _truncated version of the tests. All tests pass on the new code. Bug: 27324359 (cherry picked from commit 626efb78a6e1f0b2d637368f1eba175cfe89fb1c) Change-Id: Iba60a926cf5a1d6b517a6bfd8c797d724f093010 --- libcutils/tests/Android.mk | 3 +- libcutils/tests/trace-dev_test.cpp | 295 +++++++++++++++++++++++++++++ libcutils/trace-dev.c | 50 +++-- 3 files changed, 321 insertions(+), 27 deletions(-) create mode 100644 libcutils/tests/trace-dev_test.cpp diff --git a/libcutils/tests/Android.mk b/libcutils/tests/Android.mk index 4da5ed6f0..52cf5f407 100644 --- a/libcutils/tests/Android.mk +++ b/libcutils/tests/Android.mk @@ -23,8 +23,9 @@ test_src_files_nonwindows := \ test_target_only_src_files := \ MemsetTest.cpp \ PropertiesTest.cpp \ + trace-dev_test.cpp \ -test_libraries := libcutils liblog +test_libraries := libcutils liblog libbase # diff --git a/libcutils/tests/trace-dev_test.cpp b/libcutils/tests/trace-dev_test.cpp new file mode 100644 index 000000000..edf981b39 --- /dev/null +++ b/libcutils/tests/trace-dev_test.cpp @@ -0,0 +1,295 @@ +/* + * Copyright (C) 2016 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 +#include + +#include +#include + +#include +#include +#include +#include + +#include "../trace-dev.c" + +class TraceDevTest : public ::testing::Test { + protected: + void SetUp() override { + lseek(tmp_file_.fd, 0, SEEK_SET); + atrace_marker_fd = tmp_file_.fd; + } + + void TearDown() override { + atrace_marker_fd = -1; + } + + TemporaryFile tmp_file_; + + static std::string MakeName(size_t length) { + std::string name; + for (size_t i = 0; i < length; i++) { + name += '0' + (i % 10); + } + return name; + } +}; + +TEST_F(TraceDevTest, atrace_begin_body_normal) { + atrace_begin_body("fake_name"); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("B|%d|fake_name", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_begin_body_exact) { + std::string expected = android::base::StringPrintf("B|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 1); + atrace_begin_body(name.c_str()); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_begin_body(name.c_str()); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_begin_body_truncated) { + std::string expected = android::base::StringPrintf("B|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_begin_body(name.c_str()); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 1; + expected += android::base::StringPrintf("%.*s", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_begin_body_normal) { + atrace_async_begin_body("fake_name", 12345); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("S|%d|fake_name|12345", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_begin_body_exact) { + std::string expected = android::base::StringPrintf("S|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 7); + atrace_async_begin_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|12345"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_async_begin_body(name.c_str(), 12345); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_begin_body_truncated) { + std::string expected = android::base::StringPrintf("S|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_async_begin_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 7; + expected += android::base::StringPrintf("%.*s|12345", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_end_body_normal) { + atrace_async_end_body("fake_name", 12345); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("F|%d|fake_name|12345", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_end_body_exact) { + std::string expected = android::base::StringPrintf("F|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 7); + atrace_async_end_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|12345"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_async_end_body(name.c_str(), 12345); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_end_body_truncated) { + std::string expected = android::base::StringPrintf("F|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_async_end_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 7; + expected += android::base::StringPrintf("%.*s|12345", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int_body_normal) { + atrace_int_body("fake_name", 12345); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("C|%d|fake_name|12345", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int_body_exact) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 7); + atrace_int_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|12345"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_int_body(name.c_str(), 12345); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int_body_truncated) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_int_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 7; + expected += android::base::StringPrintf("%.*s|12345", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int64_body_normal) { + atrace_int64_body("fake_name", 17179869183L); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("C|%d|fake_name|17179869183", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int64_body_exact) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 13); + atrace_int64_body(name.c_str(), 17179869183L); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|17179869183"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_int64_body(name.c_str(), 17179869183L); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int64_body_truncated) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_int64_body(name.c_str(), 17179869183L); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 13; + expected += android::base::StringPrintf("%.*s|17179869183", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} diff --git a/libcutils/trace-dev.c b/libcutils/trace-dev.c index f025256f1..5df1c5a90 100644 --- a/libcutils/trace-dev.c +++ b/libcutils/trace-dev.c @@ -194,49 +194,47 @@ void atrace_setup() void atrace_begin_body(const char* name) { char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "B|%d|%s", getpid(), name); + int len = snprintf(buf, sizeof(buf), "B|%d|%s", getpid(), name); + if (len >= (int) sizeof(buf)) { + ALOGW("Truncated name in %s: %s\n", __FUNCTION__, name); + len = sizeof(buf) - 1; + } write(atrace_marker_fd, buf, len); } +#define WRITE_MSG(format_begin, format_end, pid, name, value) { \ + char buf[ATRACE_MESSAGE_LENGTH]; \ + int len = snprintf(buf, sizeof(buf), format_begin "%s" format_end, pid, \ + name, value); \ + if (len >= (int) sizeof(buf)) { \ + /* Given the sizeof(buf), and all of the current format buffers, \ + * it is impossible for name_len to be < 0 if len >= sizeof(buf). */ \ + int name_len = strlen(name) - (len - sizeof(buf)) - 1; \ + /* Truncate the name to make the message fit. */ \ + ALOGW("Truncated name in %s: %s\n", __FUNCTION__, name); \ + len = snprintf(buf, sizeof(buf), format_begin "%.*s" format_end, pid, \ + name_len, name, value); \ + } \ + write(atrace_marker_fd, buf, len); \ +} void atrace_async_begin_body(const char* name, int32_t cookie) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "S|%d|%s|%" PRId32, - getpid(), name, cookie); - write(atrace_marker_fd, buf, len); + WRITE_MSG("S|%d|", "|%" PRId32, getpid(), name, cookie); } void atrace_async_end_body(const char* name, int32_t cookie) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "F|%d|%s|%" PRId32, - getpid(), name, cookie); - write(atrace_marker_fd, buf, len); + WRITE_MSG("F|%d|", "|%" PRId32, getpid(), name, cookie); } void atrace_int_body(const char* name, int32_t value) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "C|%d|%s|%" PRId32, - getpid(), name, value); - write(atrace_marker_fd, buf, len); + WRITE_MSG("C|%d|", "|%" PRId32, getpid(), name, value); } void atrace_int64_body(const char* name, int64_t value) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "C|%d|%s|%" PRId64, - getpid(), name, value); - write(atrace_marker_fd, buf, len); + WRITE_MSG("C|%d|", "|%" PRId64, getpid(), name, value); } From 953eddec4d7c0437a6920973d335d6abcb338c97 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 16 Mar 2016 13:39:38 -0700 Subject: [PATCH 08/19] debuggerd: kill crashing processes with the signal they died with. Bug: http://b/27675306 Change-Id: I951c5d7e54c35d88c65c5dc856e0b9d5a93d47b2 (cherry picked from commit 561497c0a8073d8e04b387be0e0aa995424cee59) --- debuggerd/debuggerd.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 86d7c14d5..11bc9f65e 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -374,7 +374,8 @@ static void ptrace_siblings(pid_t pid, pid_t main_tid, std::set& tids) { } static bool perform_dump(const debugger_request_t& request, int fd, int tombstone_fd, - BacktraceMap* backtrace_map, const std::set& siblings) { + BacktraceMap* backtrace_map, const std::set& siblings, + int* crash_signal) { if (TEMP_FAILURE_RETRY(write(fd, "\0", 1)) != 1) { ALOGE("debuggerd: failed to respond to client: %s\n", strerror(errno)); return false; @@ -420,6 +421,7 @@ static bool perform_dump(const debugger_request_t& request, int fd, int tombston // the non-signaled threads stop moving. Without // this we get a lot of "ptrace detach failed: // No such process". + *crash_signal = signal; kill(request.pid, SIGSTOP); engrave_tombstone(tombstone_fd, backtrace_map, request.pid, request.tid, siblings, signal, request.original_si_code, request.abort_msg_address); @@ -632,7 +634,8 @@ static void handle_request(int fd) { _exit(1); } - succeeded = perform_dump(request, fd, tombstone_fd, backtrace_map.get(), siblings); + int crash_signal = SIGKILL; + succeeded = perform_dump(request, fd, tombstone_fd, backtrace_map.get(), siblings, &crash_signal); if (succeeded) { if (request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) { if (!tombstone_path.empty()) { @@ -659,9 +662,7 @@ static void handle_request(int fd) { // Send the signal back to the process if it crashed and we're not waiting for gdb. if (!attach_gdb && request.action == DEBUGGER_ACTION_CRASH) { - // TODO: Send the same signal that triggered the dump, so that shell says "Segmentation fault" - // instead of "Killed"? - if (!send_signal(SIGKILL)) { + if (!send_signal(crash_signal)) { ALOGE("debuggerd: failed to kill process %d: %s", request.pid, strerror(errno)); } } From 6953a4edcdf1d3d1e6a768844bfb4ac645d322dc Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 19 Feb 2016 10:41:17 -0800 Subject: [PATCH 09/19] Remove dead code from debuggerd. system/core/debuggerd/debuggerd.cpp:683:5: warning: Value stored to 'logsocket' is never read logsocket = -1; ^ ~~ Bug: http://b/27264392 Change-Id: I8eab8a02b67f219c32aea49e4d4957e5642df38f (cherry picked from commit 6da1353863dea7ed6835bd776a0cf4a49d12f910) --- debuggerd/debuggerd.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 11bc9f65e..c78698ae3 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -703,13 +703,6 @@ static int do_server() { // Ignore failed writes to closed sockets signal(SIGPIPE, SIG_IGN); - int logsocket = socket_local_client("logd", ANDROID_SOCKET_NAMESPACE_ABSTRACT, SOCK_DGRAM); - if (logsocket < 0) { - logsocket = -1; - } else { - fcntl(logsocket, F_SETFD, FD_CLOEXEC); - } - struct sigaction act; act.sa_handler = SIG_DFL; sigemptyset(&act.sa_mask); From 058677073c4709eb7075991a39d957b66e553ef3 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 19 Feb 2016 18:13:02 -0800 Subject: [PATCH 10/19] Clean up CLOEXEC in debuggerd. Change-Id: I1cd75f6a8f98e99f4a4fedfc706103ce34035765 (cherry picked from commit 17ba68d0cde001c2e73a310ee9a895a5b3bb5d32) --- debuggerd/debuggerd.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index c78698ae3..eabbb9abf 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -710,10 +710,9 @@ static int do_server() { act.sa_flags = SA_NOCLDWAIT; sigaction(SIGCHLD, &act, 0); - int s = socket_local_server(SOCKET_NAME, ANDROID_SOCKET_NAMESPACE_ABSTRACT, SOCK_STREAM); - if (s < 0) - return 1; - fcntl(s, F_SETFD, FD_CLOEXEC); + int s = socket_local_server(SOCKET_NAME, ANDROID_SOCKET_NAMESPACE_ABSTRACT, + SOCK_STREAM | SOCK_CLOEXEC); + if (s == -1) return 1; ALOGI("debuggerd: starting\n"); @@ -723,14 +722,12 @@ static int do_server() { socklen_t alen = sizeof(ss); ALOGV("waiting for connection\n"); - int fd = accept(s, addrp, &alen); - if (fd < 0) { - ALOGV("accept failed: %s\n", strerror(errno)); + int fd = accept4(s, addrp, &alen, SOCK_CLOEXEC); + if (fd == -1) { + ALOGE("accept failed: %s\n", strerror(errno)); continue; } - fcntl(fd, F_SETFD, FD_CLOEXEC); - handle_request(fd); } return 0; From e56901ef8db100c67e0549ee30284f7c1b89374d Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 16 Mar 2016 18:09:15 -0700 Subject: [PATCH 11/19] debuggerd: fork the signal sender once. Bug: http://b/27427439 Change-Id: I6294ff68a150bc9950a300264c31d2141307ac66 (cherry picked from commit f5e8f0b9cd9ec0214a6f9cd38dd6d9af3268f9aa) --- debuggerd/Android.mk | 1 + debuggerd/debuggerd.cpp | 105 ++++--------------------- debuggerd/signal_sender.cpp | 152 ++++++++++++++++++++++++++++++++++++ debuggerd/signal_sender.h | 30 +++++++ 4 files changed, 197 insertions(+), 91 deletions(-) create mode 100644 debuggerd/signal_sender.cpp create mode 100644 debuggerd/signal_sender.h diff --git a/debuggerd/Android.mk b/debuggerd/Android.mk index 9e4f1f7d2..6469db451 100644 --- a/debuggerd/Android.mk +++ b/debuggerd/Android.mk @@ -15,6 +15,7 @@ LOCAL_SRC_FILES:= \ debuggerd.cpp \ elf_utils.cpp \ getevent.cpp \ + signal_sender.cpp \ tombstone.cpp \ utility.cpp \ diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index eabbb9abf..b6c2f8a18 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -15,21 +15,20 @@ */ #include +#include #include #include #include #include #include #include -#include -#include - -#include #include #include #include #include +#include #include +#include #include @@ -48,6 +47,7 @@ #include "backtrace.h" #include "getevent.h" +#include "signal_sender.h" #include "tombstone.h" #include "utility.h" @@ -422,7 +422,7 @@ static bool perform_dump(const debugger_request_t& request, int fd, int tombston // this we get a lot of "ptrace detach failed: // No such process". *crash_signal = signal; - kill(request.pid, SIGSTOP); + send_signal(request.pid, 0, SIGSTOP); engrave_tombstone(tombstone_fd, backtrace_map, request.pid, request.tid, siblings, signal, request.original_si_code, request.abort_msg_address); break; @@ -451,60 +451,6 @@ static bool drop_privileges() { return true; } -// Fork a process that listens for signals to send, or 0, to exit. -static bool fork_signal_sender(int* out_fd, pid_t* sender_pid, pid_t target_pid) { - int sfd[2]; - if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, sfd) != 0) { - ALOGE("debuggerd: failed to create socketpair for signal sender: %s", strerror(errno)); - return false; - } - - pid_t fork_pid = fork(); - if (fork_pid == -1) { - ALOGE("debuggerd: failed to initialize signal sender: fork failed: %s", strerror(errno)); - return false; - } else if (fork_pid == 0) { - close(sfd[1]); - - while (true) { - int signal; - int rc = TEMP_FAILURE_RETRY(read(sfd[0], &signal, sizeof(signal))); - if (rc < 0) { - ALOGE("debuggerd: signal sender failed to read from socket"); - kill(target_pid, SIGKILL); - exit(1); - } else if (rc != sizeof(signal)) { - ALOGE("debuggerd: signal sender read unexpected number of bytes: %d", rc); - kill(target_pid, SIGKILL); - exit(1); - } - - // Report success after sending a signal, or before exiting. - int err = 0; - if (signal != 0) { - if (kill(target_pid, signal) != 0) { - err = errno; - } - } - - if (TEMP_FAILURE_RETRY(write(sfd[0], &err, sizeof(err))) < 0) { - ALOGE("debuggerd: signal sender failed to write: %s", strerror(errno)); - kill(target_pid, SIGKILL); - exit(1); - } - - if (signal == 0) { - exit(0); - } - } - } else { - close(sfd[0]); - *out_fd = sfd[1]; - *sender_pid = fork_pid; - return true; - } -} - static void handle_request(int fd) { ALOGV("handle_request(%d)\n", fd); @@ -585,15 +531,6 @@ static void handle_request(int fd) { // Don't attach to the sibling threads if we want to attach gdb. // Supposedly, it makes the process less reliable. bool attach_gdb = should_attach_gdb(&request); - int signal_fd = -1; - pid_t signal_pid = 0; - - // Fork a process that stays root, and listens on a pipe to pause and resume the target. - if (!fork_signal_sender(&signal_fd, &signal_pid, request.pid)) { - ALOGE("debuggerd: failed to fork signal sender"); - exit(1); - } - if (attach_gdb) { // Open all of the input devices we need to listen for VOLUMEDOWN before dropping privileges. if (init_getevent() != 0) { @@ -603,21 +540,6 @@ static void handle_request(int fd) { } - auto send_signal = [=](int signal) { - int error; - if (TEMP_FAILURE_RETRY(write(signal_fd, &signal, sizeof(signal))) < 0) { - ALOGE("debuggerd: failed to notify signal process: %s", strerror(errno)); - return false; - } else if (TEMP_FAILURE_RETRY(read(signal_fd, &error, sizeof(error))) < 0) { - ALOGE("debuggerd: failed to read response from signal process: %s", strerror(errno)); - return false; - } else if (error != 0) { - errno = error; - return false; - } - return true; - }; - std::set siblings; if (!attach_gdb) { ptrace_siblings(request.pid, request.tid, siblings); @@ -646,7 +568,7 @@ static void handle_request(int fd) { if (attach_gdb) { // Tell the signal process to send SIGSTOP to the target. - if (!send_signal(SIGSTOP)) { + if (!send_signal(request.pid, 0, SIGSTOP)) { ALOGE("debuggerd: failed to stop process for gdb attach: %s", strerror(errno)); attach_gdb = false; } @@ -662,7 +584,7 @@ static void handle_request(int fd) { // Send the signal back to the process if it crashed and we're not waiting for gdb. if (!attach_gdb && request.action == DEBUGGER_ACTION_CRASH) { - if (!send_signal(crash_signal)) { + if (!send_signal(request.pid, request.tid, crash_signal)) { ALOGE("debuggerd: failed to kill process %d: %s", request.pid, strerror(errno)); } } @@ -672,18 +594,13 @@ static void handle_request(int fd) { wait_for_user_action(request); // Tell the signal process to send SIGCONT to the target. - if (!send_signal(SIGCONT)) { + if (!send_signal(request.pid, 0, SIGCONT)) { ALOGE("debuggerd: failed to resume process %d: %s", request.pid, strerror(errno)); } uninit_getevent(); } - if (!send_signal(0)) { - ALOGE("debuggerd: failed to notify signal sender to finish"); - kill(signal_pid, SIGKILL); - } - waitpid(signal_pid, nullptr, 0); exit(!succeeded); } @@ -714,6 +631,12 @@ static int do_server() { SOCK_STREAM | SOCK_CLOEXEC); if (s == -1) return 1; + // Fork a process that stays root, and listens on a pipe to pause and resume the target. + if (!start_signal_sender()) { + ALOGE("debuggerd: failed to fork signal sender"); + return 1; + } + ALOGI("debuggerd: starting\n"); for (;;) { diff --git a/debuggerd/signal_sender.cpp b/debuggerd/signal_sender.cpp new file mode 100644 index 000000000..35e0c0b57 --- /dev/null +++ b/debuggerd/signal_sender.cpp @@ -0,0 +1,152 @@ +/* + * Copyright (C) 2016 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 +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "signal_sender.h" + +static int signal_fd = -1; +static pid_t signal_pid; +struct signal_message { + pid_t pid; + pid_t tid; + int signal; +}; + +// Fork a process to send signals for the worker processes to use after they've dropped privileges. +bool start_signal_sender() { + if (signal_pid == 0) { + ALOGE("debuggerd: attempted to start signal sender multiple times"); + return false; + } + + int sfd[2]; + if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sfd) != 0) { + ALOGE("debuggerd: failed to create socketpair for signal sender: %s", strerror(errno)); + return false; + } + + pid_t parent = getpid(); + pid_t fork_pid = fork(); + if (fork_pid == -1) { + ALOGE("debuggerd: failed to initialize signal sender: fork failed: %s", strerror(errno)); + return false; + } else if (fork_pid == 0) { + close(sfd[1]); + + while (true) { + signal_message msg; + int rc = TEMP_FAILURE_RETRY(read(sfd[0], &msg, sizeof(msg))); + if (rc < 0) { + ALOGE("debuggerd: signal sender failed to read from socket"); + break; + } else if (rc != sizeof(msg)) { + ALOGE("debuggerd: signal sender read unexpected number of bytes: %d", rc); + break; + } + + // Report success after sending a signal + int err = 0; + if (msg.tid > 0) { + if (syscall(SYS_tgkill, msg.pid, msg.tid, msg.signal) != 0) { + err = errno; + } + } else { + if (kill(msg.pid, msg.signal) != 0) { + err = errno; + } + } + + if (TEMP_FAILURE_RETRY(write(sfd[0], &err, sizeof(err))) < 0) { + ALOGE("debuggerd: signal sender failed to write: %s", strerror(errno)); + } + } + + // Our parent proably died, but if not, kill them. + if (getppid() == parent) { + kill(parent, SIGKILL); + } + _exit(1); + } else { + close(sfd[0]); + signal_fd = sfd[1]; + signal_pid = fork_pid; + return true; + } +} + +bool stop_signal_sender() { + if (signal_pid <= 0) { + return false; + } + + if (kill(signal_pid, SIGKILL) != 0) { + ALOGE("debuggerd: failed to kill signal sender: %s", strerror(errno)); + return false; + } + + close(signal_fd); + signal_fd = -1; + + int status; + waitpid(signal_pid, &status, 0); + signal_pid = 0; + + return true; +} + +bool send_signal(pid_t pid, pid_t tid, int signal) { + if (signal_fd == -1) { + ALOGE("debuggerd: attempted to send signal before signal sender was started"); + errno = EHOSTUNREACH; + return false; + } + + signal_message msg = {.pid = pid, .tid = tid, .signal = signal }; + if (TEMP_FAILURE_RETRY(write(signal_fd, &msg, sizeof(msg))) < 0) { + ALOGE("debuggerd: failed to send message to signal sender: %s", strerror(errno)); + errno = EHOSTUNREACH; + return false; + } + + int response; + ssize_t rc = TEMP_FAILURE_RETRY(read(signal_fd, &response, sizeof(response))); + if (rc == 0) { + ALOGE("debuggerd: received EOF from signal sender"); + errno = EHOSTUNREACH; + return false; + } else if (rc < 0) { + ALOGE("debuggerd: failed to receive response from signal sender: %s", strerror(errno)); + errno = EHOSTUNREACH; + return false; + } + + if (response == 0) { + return true; + } + + errno = response; + return false; +} diff --git a/debuggerd/signal_sender.h b/debuggerd/signal_sender.h new file mode 100644 index 000000000..0443272e6 --- /dev/null +++ b/debuggerd/signal_sender.h @@ -0,0 +1,30 @@ +/* + * Copyright (C) 2016 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. + */ + +#ifndef _DEBUGGERD_SIGNAL_SENDER_H +#define _DEBUGGERD_SIGNAL_SENDER_H + +#include + +bool start_signal_sender(); +bool stop_signal_sender(); + +// Sends a signal to a target process or thread. +// If tid is greater than zero, this performs tgkill(pid, tid, signal). +// Otherwise, it performs kill(pid, signal). +bool send_signal(pid_t pid, pid_t tid, int signal); + +#endif From c6054a8114cefb43d9a93600b2c124a76eeefa4a Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 16 Mar 2016 20:19:44 -0700 Subject: [PATCH 12/19] debuggerd: monitor the worker process for failure. Use sigtimedwait on SIGCHLD to watch our forked worker processes for failure, so that we can guarantee that we always resume/kill the target process if libunwind crashes. Bug: http://b/27427439 Change-Id: I5a5da1f1abd7dc9d01223f5b3778e946e2d47d20 (cherry picked from commit 630bc80e185f0c596a15822b67c62a0ecd6c982c) --- debuggerd/debuggerd.cpp | 156 ++++++++++++++++++++++++++++------------ 1 file changed, 111 insertions(+), 45 deletions(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index b6c2f8a18..71c1e83da 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -451,45 +451,7 @@ static bool drop_privileges() { return true; } -static void handle_request(int fd) { - ALOGV("handle_request(%d)\n", fd); - - ScopedFd closer(fd); - debugger_request_t request; - memset(&request, 0, sizeof(request)); - int status = read_request(fd, &request); - if (status != 0) { - return; - } - - ALOGV("BOOM: pid=%d uid=%d gid=%d tid=%d\n", request.pid, request.uid, request.gid, request.tid); - -#if defined(__LP64__) - // On 64 bit systems, requests to dump 32 bit and 64 bit tids come - // to the 64 bit debuggerd. If the process is a 32 bit executable, - // redirect the request to the 32 bit debuggerd. - if (is32bit(request.tid)) { - // Only dump backtrace and dump tombstone requests can be redirected. - if (request.action == DEBUGGER_ACTION_DUMP_BACKTRACE || - request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) { - redirect_to_32(fd, &request); - } else { - ALOGE("debuggerd: Not allowed to redirect action %d to 32 bit debuggerd\n", request.action); - } - return; - } -#endif - - // Fork a child to handle the rest of the request. - pid_t fork_pid = fork(); - if (fork_pid == -1) { - ALOGE("debuggerd: failed to fork: %s\n", strerror(errno)); - return; - } else if (fork_pid != 0) { - waitpid(fork_pid, nullptr, 0); - return; - } - +static void worker_process(int fd, debugger_request_t& request) { // Open the tombstone file if we need it. std::string tombstone_path; int tombstone_fd = -1; @@ -604,6 +566,111 @@ static void handle_request(int fd) { exit(!succeeded); } +static void monitor_worker_process(int child_pid, const debugger_request_t& request) { + struct timespec timeout = {.tv_sec = 10, .tv_nsec = 0 }; + + sigset_t signal_set; + sigemptyset(&signal_set); + sigaddset(&signal_set, SIGCHLD); + + bool kill_worker = false; + bool kill_target = false; + bool kill_self = false; + + int status; + siginfo_t siginfo; + int signal = TEMP_FAILURE_RETRY(sigtimedwait(&signal_set, &siginfo, &timeout)); + if (signal == SIGCHLD) { + pid_t rc = waitpid(0, &status, WNOHANG | WUNTRACED); + if (rc != child_pid) { + ALOGE("debuggerd: waitpid returned unexpected pid (%d), committing murder-suicide", rc); + kill_worker = true; + kill_target = true; + kill_self = true; + } + + if (WIFSIGNALED(status)) { + ALOGE("debuggerd: worker process %d terminated due to signal %d", child_pid, WTERMSIG(status)); + kill_worker = false; + kill_target = true; + } else if (WIFSTOPPED(status)) { + ALOGE("debuggerd: worker process %d stopped due to signal %d", child_pid, WSTOPSIG(status)); + kill_worker = true; + kill_target = true; + } + } else { + ALOGE("debuggerd: worker process %d timed out", child_pid); + kill_worker = true; + kill_target = true; + } + + if (kill_worker) { + // Something bad happened, kill the worker. + if (kill(child_pid, SIGKILL) != 0) { + ALOGE("debuggerd: failed to kill worker process %d: %s", child_pid, strerror(errno)); + } else { + waitpid(child_pid, &status, 0); + } + } + + if (kill_target) { + // Resume or kill the target, depending on what the initial request was. + if (request.action == DEBUGGER_ACTION_CRASH) { + ALOGE("debuggerd: killing target %d", request.pid); + kill(request.pid, SIGKILL); + } else { + ALOGE("debuggerd: resuming target %d", request.pid); + kill(request.pid, SIGCONT); + } + } + + if (kill_self) { + stop_signal_sender(); + _exit(1); + } +} + +static void handle_request(int fd) { + ALOGV("handle_request(%d)\n", fd); + + ScopedFd closer(fd); + debugger_request_t request; + memset(&request, 0, sizeof(request)); + int status = read_request(fd, &request); + if (status != 0) { + return; + } + + ALOGW("debuggerd: handling request: pid=%d uid=%d gid=%d tid=%d\n", request.pid, request.uid, + request.gid, request.tid); + +#if defined(__LP64__) + // On 64 bit systems, requests to dump 32 bit and 64 bit tids come + // to the 64 bit debuggerd. If the process is a 32 bit executable, + // redirect the request to the 32 bit debuggerd. + if (is32bit(request.tid)) { + // Only dump backtrace and dump tombstone requests can be redirected. + if (request.action == DEBUGGER_ACTION_DUMP_BACKTRACE || + request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) { + redirect_to_32(fd, &request); + } else { + ALOGE("debuggerd: Not allowed to redirect action %d to 32 bit debuggerd\n", request.action); + } + return; + } +#endif + + // Fork a child to handle the rest of the request. + pid_t fork_pid = fork(); + if (fork_pid == -1) { + ALOGE("debuggerd: failed to fork: %s\n", strerror(errno)); + } else if (fork_pid == 0) { + worker_process(fd, request); + } else { + monitor_worker_process(fork_pid, request); + } +} + static int do_server() { // debuggerd crashes can't be reported to debuggerd. // Reset all of the crash handlers. @@ -620,12 +687,11 @@ static int do_server() { // Ignore failed writes to closed sockets signal(SIGPIPE, SIG_IGN); - struct sigaction act; - act.sa_handler = SIG_DFL; - sigemptyset(&act.sa_mask); - sigaddset(&act.sa_mask,SIGCHLD); - act.sa_flags = SA_NOCLDWAIT; - sigaction(SIGCHLD, &act, 0); + // Block SIGCHLD so we can sigtimedwait for it. + sigset_t sigchld; + sigemptyset(&sigchld); + sigaddset(&sigchld, SIGCHLD); + sigprocmask(SIG_SETMASK, &sigchld, nullptr); int s = socket_local_server(SOCKET_NAME, ANDROID_SOCKET_NAMESPACE_ABSTRACT, SOCK_STREAM | SOCK_CLOEXEC); From 2d995ba3c9774ced1cf09bfdc8db8a15267055fd Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 17 Mar 2016 13:21:12 -0700 Subject: [PATCH 13/19] debuggerd: fix stupid typo. Change-Id: Icd9a25a71e1e8580a200fe68bce0b17d09c51642 --- debuggerd/signal_sender.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debuggerd/signal_sender.cpp b/debuggerd/signal_sender.cpp index 35e0c0b57..1cfb70407 100644 --- a/debuggerd/signal_sender.cpp +++ b/debuggerd/signal_sender.cpp @@ -37,7 +37,7 @@ struct signal_message { // Fork a process to send signals for the worker processes to use after they've dropped privileges. bool start_signal_sender() { - if (signal_pid == 0) { + if (signal_pid != 0) { ALOGE("debuggerd: attempted to start signal sender multiple times"); return false; } From d483c4968bf51996588337476b0f7ad82467a74b Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 17 Mar 2016 13:39:15 -0700 Subject: [PATCH 14/19] debuggerd: don't send SIGSTOP to crashing processes. This was actually nonfunctional until f5e8f0b, because it was using kill after privileges were dropped. This doesn't seem necessary after the changes to the sibling thread ptrace logic, though. Bug: http://b/27427439 Change-Id: I6bffbc14e0cf5e377bbfa39c945518e0d436c223 (cherry picked from commit b17f228ff6cdc73f0ca3ab4578f78faf1a7f1b86) --- debuggerd/debuggerd.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 71c1e83da..fff28802a 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -417,12 +417,7 @@ static bool perform_dump(const debugger_request_t& request, int fd, int tombston #endif case SIGTRAP: ALOGV("stopped -- fatal signal\n"); - // Send a SIGSTOP to the process to make all of - // the non-signaled threads stop moving. Without - // this we get a lot of "ptrace detach failed: - // No such process". *crash_signal = signal; - send_signal(request.pid, 0, SIGSTOP); engrave_tombstone(tombstone_fd, backtrace_map, request.pid, request.tid, siblings, signal, request.original_si_code, request.abort_msg_address); break; From 9261e6e120bcc93f1d4bb82af853e5cdff799770 Mon Sep 17 00:00:00 2001 From: Martijn Coenen Date: Fri, 18 Mar 2016 15:28:31 +0100 Subject: [PATCH 15/19] Don't use mem cgroups for pid accounting. Commit b82bab66 introduced the use of memory cgroups for keeping track of forked PIDs; it basically creates a separate memory cgroup for every process forked from zygote. Each such memory cgroup which also have its own LRU with (in)active file and anonymous pages. The current theory is this could potentially introduce two problems: 1) kswapd runs longer because it has to iterate over the LRUs of all mem cgroups, instead of over the LRUs of a single root mem cgroup; 2) the way kswapd reclaims things will be different also - I think it will tend to bias reclaim to smaller mem cgroups, and process private pages will end up on ZRAM swap much sooner. Until we figure this out, fall back to the CPU accounting cgroup for keeping track of forked PIDs. This leaves us with a single root mem cgroup again. We can also keep userspace lmkd enabled because it only requires the root mem cgroup. Bug: 27381069 Change-Id: Ife397a6ac232761f2adfe6f5056582be0d1b4ff1 --- libprocessgroup/processgroup.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index 5ab957d86..1bc1659e6 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -37,6 +37,9 @@ #include +// Uncomment line below use memory cgroups for keeping track of (forked) PIDs +// #define USE_MEMCG 1 + #define MEM_CGROUP_PATH "/dev/memcg/apps" #define MEM_CGROUP_TASKS "/dev/memcg/apps/tasks" #define ACCT_CGROUP_PATH "/acct" @@ -67,6 +70,7 @@ struct ctx { }; static const char* getCgroupRootPath() { +#ifdef USE_MEMCG static const char* cgroup_root_path = NULL; std::call_once(init_path_flag, [&]() { // Check if mem cgroup is mounted, only then check for write-access to avoid @@ -75,6 +79,9 @@ static const char* getCgroupRootPath() { ACCT_CGROUP_PATH : MEM_CGROUP_PATH; }); return cgroup_root_path; +#else + return ACCT_CGROUP_PATH; +#endif } static int convertUidToPath(char *path, size_t size, uid_t uid) From a471ff0a13c84270fc9b4ca98f0517292e905b7d Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 22 Mar 2016 16:37:45 -0700 Subject: [PATCH 16/19] debuggerd: always send SIGCONT after detaching. Bug: http://b/27330889 Change-Id: I104248af1cde03dbdbacc03c87fe7e2dffd6c037 (cherry picked from commit 24464185eb260b4af577895e1c2d35b734a7f99b) --- debuggerd/debuggerd.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 97f40963c..18c608b1c 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -586,9 +586,7 @@ static void monitor_worker_process(int child_pid, const debugger_request_t& requ kill_worker = true; kill_target = true; kill_self = true; - } - - if (WIFSIGNALED(status)) { + } else if (WIFSIGNALED(status)) { ALOGE("debuggerd: worker process %d terminated due to signal %d", child_pid, WTERMSIG(status)); kill_worker = false; kill_target = true; @@ -612,15 +610,16 @@ static void monitor_worker_process(int child_pid, const debugger_request_t& requ } } - if (kill_target) { - // Resume or kill the target, depending on what the initial request was. - if (request.action == DEBUGGER_ACTION_CRASH) { - ALOGE("debuggerd: killing target %d", request.pid); - kill(request.pid, SIGKILL); - } else { - ALOGE("debuggerd: resuming target %d", request.pid); - kill(request.pid, SIGCONT); - } + int exit_signal = SIGCONT; + if (kill_target && request.action == DEBUGGER_ACTION_CRASH) { + ALOGE("debuggerd: killing target %d", request.pid); + exit_signal = SIGKILL; + } else { + ALOGW("debuggerd: resuming target %d", request.pid); + } + + if (kill(request.pid, exit_signal) != 0) { + ALOGE("debuggerd: failed to send signal %d to target: %s", exit_signal, strerror(errno)); } if (kill_self) { From dff97e4a7f05219ddb9ca2f6c5afb4ac9586d6f2 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 23 Mar 2016 14:02:52 -0700 Subject: [PATCH 17/19] debuggerd: waitpid for all children, and log the result. Change-Id: Ic575e6db76ab153b4b238589a8cd299812d0e046 (cherry picked from commit 280800552165b4685fbe86fd8e9dc3b9d9b062cb) --- debuggerd/debuggerd.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 18c608b1c..842bf0968 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -580,9 +580,20 @@ static void monitor_worker_process(int child_pid, const debugger_request_t& requ siginfo_t siginfo; int signal = TEMP_FAILURE_RETRY(sigtimedwait(&signal_set, &siginfo, &timeout)); if (signal == SIGCHLD) { - pid_t rc = waitpid(0, &status, WNOHANG | WUNTRACED); + pid_t rc = waitpid(-1, &status, WNOHANG | WUNTRACED); if (rc != child_pid) { ALOGE("debuggerd: waitpid returned unexpected pid (%d), committing murder-suicide", rc); + + if (WIFEXITED(status)) { + ALOGW("debuggerd: pid %d exited with status %d", rc, WEXITSTATUS(status)); + } else if (WIFSIGNALED(status)) { + ALOGW("debuggerd: pid %d received signal %d", rc, WTERMSIG(status)); + } else if (WIFSTOPPED(status)) { + ALOGW("debuggerd: pid %d stopped by signal %d", rc, WSTOPSIG(status)); + } else if (WIFCONTINUED(status)) { + ALOGW("debuggerd: pid %d continued", rc); + } + kill_worker = true; kill_target = true; kill_self = true; From 1b6d0dc731629c8ad05dc67fe848ebbae4792fe7 Mon Sep 17 00:00:00 2001 From: Mark Salyzyn Date: Thu, 31 Mar 2016 16:03:22 +0000 Subject: [PATCH 18/19] Revert "sdcard: Support sdcardfs" This reverts commit 2bd0efa89cf93bf055bd91b1641477da2937ce8d. Bug: 27932087 Change-Id: Ie27f17c1f283514b90ce9da0c895b528d87e5f47 --- sdcard/sdcard.c | 105 +----------------------------------------------- 1 file changed, 1 insertion(+), 104 deletions(-) diff --git a/sdcard/sdcard.c b/sdcard/sdcard.c index d8fda676f..45efe369e 100644 --- a/sdcard/sdcard.c +++ b/sdcard/sdcard.c @@ -1894,105 +1894,6 @@ static void run(const char* source_path, const char* label, uid_t uid, exit(1); } -static int sdcardfs_setup(const char *source_path, const char *dest_path, uid_t fsuid, - gid_t fsgid, bool multi_user, userid_t userid, gid_t gid, mode_t mask) { - char opts[256]; - - snprintf(opts, sizeof(opts), - "fsuid=%d,fsgid=%d,%smask=%d,userid=%d,gid=%d", - fsuid, fsgid, multi_user?"multiuser,":"", mask, userid, gid); - - if (mount(source_path, dest_path, "sdcardfs", - MS_NOSUID | MS_NODEV | MS_NOEXEC | MS_NOATIME, opts) != 0) { - ERROR("failed to mount sdcardfs filesystem: %s\n", strerror(errno)); - return -1; - } - - return 0; -} - -static void run_sdcardfs(const char* source_path, const char* label, uid_t uid, - gid_t gid, userid_t userid, bool multi_user, bool full_write) { - char dest_path_default[PATH_MAX]; - char dest_path_read[PATH_MAX]; - char dest_path_write[PATH_MAX]; - char obb_path[PATH_MAX]; - snprintf(dest_path_default, PATH_MAX, "/mnt/runtime/default/%s", label); - snprintf(dest_path_read, PATH_MAX, "/mnt/runtime/read/%s", label); - snprintf(dest_path_write, PATH_MAX, "/mnt/runtime/write/%s", label); - - umask(0); - if (multi_user) { - /* Multi-user storage is fully isolated per user, so "other" - * permissions are completely masked off. */ - if (sdcardfs_setup(source_path, dest_path_default, uid, gid, multi_user, userid, - AID_SDCARD_RW, 0006) - || sdcardfs_setup(source_path, dest_path_read, uid, gid, multi_user, userid, - AID_EVERYBODY, 0027) - || sdcardfs_setup(source_path, dest_path_write, uid, gid, multi_user, userid, - AID_EVERYBODY, full_write ? 0007 : 0027)) { - ERROR("failed to fuse_setup\n"); - exit(1); - } - } else { - /* Physical storage is readable by all users on device, but - * the Android directories are masked off to a single user - * deep inside attr_from_stat(). */ - if (sdcardfs_setup(source_path, dest_path_default, uid, gid, multi_user, userid, - AID_SDCARD_RW, 0006) - || sdcardfs_setup(source_path, dest_path_read, uid, gid, multi_user, userid, - AID_EVERYBODY, full_write ? 0027 : 0022) - || sdcardfs_setup(source_path, dest_path_write, uid, gid, multi_user, userid, - AID_EVERYBODY, full_write ? 0007 : 0022)) { - ERROR("failed to fuse_setup\n"); - exit(1); - } - } - - /* Drop privs */ - if (setgroups(sizeof(kGroups) / sizeof(kGroups[0]), kGroups) < 0) { - ERROR("cannot setgroups: %s\n", strerror(errno)); - exit(1); - } - if (setgid(gid) < 0) { - ERROR("cannot setgid: %s\n", strerror(errno)); - exit(1); - } - if (setuid(uid) < 0) { - ERROR("cannot setuid: %s\n", strerror(errno)); - exit(1); - } - - if (multi_user) { - snprintf(obb_path, sizeof(obb_path), "%s/obb", source_path); - fs_prepare_dir(&obb_path[0], 0775, uid, gid); - } - - exit(0); -} - -static bool supports_sdcardfs(void) { - FILE *fp; - char *buf = NULL; - size_t buflen = 0; - - fp = fopen("/proc/filesystems", "r"); - if (!fp) { - ERROR("Could not read /proc/filesystems, error: %s\n", strerror(errno)); - return false; - } - while ((getline(&buf, &buflen, fp)) > 0) { - if (strstr(buf, "sdcardfs\n")) { - free(buf); - fclose(fp); - return true; - } - } - free(buf); - fclose(fp); - return false; -} - int main(int argc, char **argv) { const char *source_path = NULL; const char *label = NULL; @@ -2065,10 +1966,6 @@ int main(int argc, char **argv) { sleep(1); } - if (supports_sdcardfs()) { - run_sdcardfs(source_path, label, uid, gid, userid, multi_user, full_write); - } else { - run(source_path, label, uid, gid, userid, multi_user, full_write); - } + run(source_path, label, uid, gid, userid, multi_user, full_write); return 1; } From f6aecf016a6231c7fe279aed47f447fea70e1e27 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 7 Apr 2016 11:05:21 -0600 Subject: [PATCH 19/19] Give users and devices control over sdcardfs. Instead of relying only on kernel support for sdcardfs, give each device the ability to quickly toggle between sdcardfs and FUSE. Also add the ability to users to explicitly enable/disable the behavior for testing and debugging purposes. Bug: 27991427 Change-Id: Ie188cb044be2ad87166f2d43c32a1f6b97660de0 --- sdcard/sdcard.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/sdcard/sdcard.c b/sdcard/sdcard.c index d8fda676f..befe38c9e 100644 --- a/sdcard/sdcard.c +++ b/sdcard/sdcard.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #include @@ -89,6 +90,9 @@ #define ERROR(x...) ALOGE(x) +#define PROP_SDCARDFS_DEVICE "ro.sys.sdcardfs" +#define PROP_SDCARDFS_USER "persist.sys.sdcardfs" + #define FUSE_UNKNOWN_INO 0xffffffff /* Maximum number of bytes to write in one request. */ @@ -1993,6 +1997,29 @@ static bool supports_sdcardfs(void) { return false; } +static bool should_use_sdcardfs(void) { + char property[PROPERTY_VALUE_MAX]; + + // Allow user to have a strong opinion about state + property_get(PROP_SDCARDFS_USER, property, ""); + if (!strcmp(property, "force_on")) { + ALOGW("User explicitly enabled sdcardfs"); + return supports_sdcardfs(); + } else if (!strcmp(property, "force_off")) { + ALOGW("User explicitly disabled sdcardfs"); + return false; + } + + // Fall back to device opinion about state + if (property_get_bool(PROP_SDCARDFS_DEVICE, false)) { + ALOGW("Device explicitly enabled sdcardfs"); + return supports_sdcardfs(); + } else { + ALOGW("Device explicitly disabled sdcardfs"); + return false; + } +} + int main(int argc, char **argv) { const char *source_path = NULL; const char *label = NULL; @@ -2065,7 +2092,7 @@ int main(int argc, char **argv) { sleep(1); } - if (supports_sdcardfs()) { + if (should_use_sdcardfs()) { run_sdcardfs(source_path, label, uid, gid, userid, multi_user, full_write); } else { run(source_path, label, uid, gid, userid, multi_user, full_write);