Merge \\"Fix sdcard logging.\\" am: 47b077a44f
am: 60b896c438
Change-Id: Ib22a4d84395378196ca191b36e3a17897360708d
diff --git a/adb/commandline.cpp b/adb/commandline.cpp
index 46a6365..eefbc83 100644
--- a/adb/commandline.cpp
+++ b/adb/commandline.cpp
@@ -1436,6 +1436,16 @@
#endif
}
+static bool _use_legacy_install() {
+ FeatureSet features;
+ std::string error;
+ if (!adb_get_feature_set(&features, &error)) {
+ fprintf(stderr, "error: %s\n", error.c_str());
+ return true;
+ }
+ return !CanUseFeature(features, kFeatureCmd);
+}
+
int adb_commandline(int argc, const char **argv) {
int no_daemon = 0;
int is_daemon = 0;
@@ -1830,17 +1840,10 @@
}
else if (!strcmp(argv[0], "install")) {
if (argc < 2) return usage();
- FeatureSet features;
- std::string error;
- if (!adb_get_feature_set(&features, &error)) {
- fprintf(stderr, "error: %s\n", error.c_str());
- return 1;
+ if (_use_legacy_install()) {
+ return install_app_legacy(transport_type, serial, argc, argv);
}
-
- if (CanUseFeature(features, kFeatureCmd)) {
- return install_app(transport_type, serial, argc, argv);
- }
- return install_app_legacy(transport_type, serial, argc, argv);
+ return install_app(transport_type, serial, argc, argv);
}
else if (!strcmp(argv[0], "install-multiple")) {
if (argc < 2) return usage();
@@ -1848,17 +1851,10 @@
}
else if (!strcmp(argv[0], "uninstall")) {
if (argc < 2) return usage();
- FeatureSet features;
- std::string error;
- if (!adb_get_feature_set(&features, &error)) {
- fprintf(stderr, "error: %s\n", error.c_str());
- return 1;
+ if (_use_legacy_install()) {
+ return uninstall_app_legacy(transport_type, serial, argc, argv);
}
-
- if (CanUseFeature(features, kFeatureCmd)) {
- return uninstall_app(transport_type, serial, argc, argv);
- }
- return uninstall_app_legacy(transport_type, serial, argc, argv);
+ return uninstall_app(transport_type, serial, argc, argv);
}
else if (!strcmp(argv[0], "sync")) {
std::string src;
@@ -2072,7 +2068,6 @@
int i;
struct stat sb;
uint64_t total_size = 0;
-
// Find all APK arguments starting at end.
// All other arguments passed through verbatim.
int first_apk = -1;
@@ -2097,7 +2092,14 @@
return 1;
}
- std::string cmd = android::base::StringPrintf("exec:pm install-create -S %" PRIu64, total_size);
+ std::string install_cmd;
+ if (_use_legacy_install()) {
+ install_cmd = "exec:pm";
+ } else {
+ install_cmd = "exec:cmd package";
+ }
+
+ std::string cmd = android::base::StringPrintf("%s install-create -S %" PRIu64, install_cmd.c_str(), total_size);
for (i = 1; i < first_apk; i++) {
cmd += " " + escape_arg(argv[i]);
}
@@ -2139,8 +2141,8 @@
}
std::string cmd = android::base::StringPrintf(
- "exec:pm install-write -S %" PRIu64 " %d %d_%s -",
- static_cast<uint64_t>(sb.st_size), session_id, i, adb_basename(file).c_str());
+ "%s install-write -S %" PRIu64 " %d %d_%s -",
+ install_cmd.c_str(), static_cast<uint64_t>(sb.st_size), session_id, i, adb_basename(file).c_str());
int localFd = adb_open(file, O_RDONLY);
if (localFd < 0) {
@@ -2175,8 +2177,8 @@
finalize_session:
// Commit session if we streamed everything okay; otherwise abandon
std::string service =
- android::base::StringPrintf("exec:pm install-%s %d",
- success ? "commit" : "abandon", session_id);
+ android::base::StringPrintf("%s install-%s %d",
+ install_cmd.c_str(), success ? "commit" : "abandon", session_id);
fd = adb_connect(service, &error);
if (fd < 0) {
fprintf(stderr, "Connect error for finalize: %s\n", error.c_str());
diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp
index a4e9cae..c352aeb 100644
--- a/debuggerd/debuggerd.cpp
+++ b/debuggerd/debuggerd.cpp
@@ -182,6 +182,16 @@
return allowed;
}
+static bool pid_contains_tid(pid_t pid, pid_t tid) {
+ char task_path[PATH_MAX];
+ if (snprintf(task_path, PATH_MAX, "/proc/%d/task/%d", pid, tid) >= PATH_MAX) {
+ ALOGE("debuggerd: task path overflow (pid = %d, tid = %d)\n", pid, tid);
+ exit(1);
+ }
+
+ return access(task_path, F_OK) == 0;
+}
+
static int read_request(int fd, debugger_request_t* out_request) {
ucred cr;
socklen_t len = sizeof(cr);
@@ -226,16 +236,13 @@
if (msg.action == DEBUGGER_ACTION_CRASH) {
// Ensure that the tid reported by the crashing process is valid.
- char buf[64];
- struct stat s;
- snprintf(buf, sizeof buf, "/proc/%d/task/%d", out_request->pid, out_request->tid);
- if (stat(buf, &s)) {
- ALOGE("tid %d does not exist in pid %d. ignoring debug request\n",
- out_request->tid, out_request->pid);
+ // This check needs to happen again after ptracing the requested thread to prevent a race.
+ if (!pid_contains_tid(out_request->pid, out_request->tid)) {
+ ALOGE("tid %d does not exist in pid %d. ignoring debug request\n", out_request->tid,
+ out_request->pid);
return -1;
}
- } else if (cr.uid == 0
- || (cr.uid == AID_SYSTEM && msg.action == DEBUGGER_ACTION_DUMP_BACKTRACE)) {
+ } else if (cr.uid == 0 || (cr.uid == AID_SYSTEM && msg.action == DEBUGGER_ACTION_DUMP_BACKTRACE)) {
// Only root or system can ask us to attach to any process and dump it explicitly.
// However, system is only allowed to collect backtraces but cannot dump tombstones.
status = get_process_info(out_request->tid, &out_request->pid,
@@ -412,10 +419,31 @@
}
#endif
-static void ptrace_siblings(pid_t pid, pid_t main_tid, std::set<pid_t>& tids) {
- char task_path[64];
+// Attach to a thread, and verify that it's still a member of the given process
+static bool ptrace_attach_thread(pid_t pid, pid_t tid) {
+ if (ptrace(PTRACE_ATTACH, tid, 0, 0) != 0) {
+ return false;
+ }
- snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);
+ // Make sure that the task we attached to is actually part of the pid we're dumping.
+ if (!pid_contains_tid(pid, tid)) {
+ if (ptrace(PTRACE_DETACH, tid, 0, 0) != 0) {
+ ALOGE("debuggerd: failed to detach from thread '%d'", tid);
+ exit(1);
+ }
+ return false;
+ }
+
+ return true;
+}
+
+static void ptrace_siblings(pid_t pid, pid_t main_tid, std::set<pid_t>& tids) {
+ char task_path[PATH_MAX];
+
+ if (snprintf(task_path, PATH_MAX, "/proc/%d/task", pid) >= PATH_MAX) {
+ ALOGE("debuggerd: task path overflow (pid = %d)\n", pid);
+ abort();
+ }
std::unique_ptr<DIR, int (*)(DIR*)> d(opendir(task_path), closedir);
@@ -442,7 +470,7 @@
continue;
}
- if (ptrace(PTRACE_ATTACH, tid, 0, 0) < 0) {
+ if (!ptrace_attach_thread(pid, tid)) {
ALOGE("debuggerd: ptrace attach to %d failed: %s", tid, strerror(errno));
continue;
}
@@ -567,11 +595,33 @@
// debugger_signal_handler().
// Attach to the target process.
- if (ptrace(PTRACE_ATTACH, request.tid, 0, 0) != 0) {
+ if (!ptrace_attach_thread(request.pid, request.tid)) {
ALOGE("debuggerd: ptrace attach failed: %s", strerror(errno));
exit(1);
}
+ // DEBUGGER_ACTION_CRASH requests can come from arbitrary processes and the tid field in the
+ // request is sent from the other side. If an attacker can cause a process to be spawned with the
+ // pid of their process, they could trick debuggerd into dumping that process by exiting after
+ // sending the request. Validate the trusted request.uid/gid to defend against this.
+ if (request.action == DEBUGGER_ACTION_CRASH) {
+ pid_t pid;
+ uid_t uid;
+ gid_t gid;
+ if (get_process_info(request.tid, &pid, &uid, &gid) != 0) {
+ ALOGE("debuggerd: failed to get process info for tid '%d'", request.tid);
+ exit(1);
+ }
+
+ if (pid != request.pid || uid != request.uid || gid != request.gid) {
+ ALOGE(
+ "debuggerd: attached task %d does not match request: "
+ "expected pid=%d,uid=%d,gid=%d, actual pid=%d,uid=%d,gid=%d",
+ request.tid, request.pid, request.uid, request.gid, pid, uid, gid);
+ exit(1);
+ }
+ }
+
// 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);
diff --git a/include/utils/Unicode.h b/include/utils/Unicode.h
index a006082..cddbab4 100644
--- a/include/utils/Unicode.h
+++ b/include/utils/Unicode.h
@@ -88,7 +88,7 @@
* "dst" becomes \xE3\x81\x82\xE3\x81\x84
* (note that "dst" is NOT null-terminated, like strncpy)
*/
-void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst);
+void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len);
/**
* Returns the unicode value at "index".
@@ -110,7 +110,7 @@
* enough to fit the UTF-16 as measured by utf16_to_utf8_length with an added
* NULL terminator.
*/
-void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst);
+void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len);
/**
* Returns the length of "src" when "src" is valid UTF-8 string.
diff --git a/libutils/String8.cpp b/libutils/String8.cpp
index ad45282..cacaf91 100644
--- a/libutils/String8.cpp
+++ b/libutils/String8.cpp
@@ -104,20 +104,21 @@
{
if (len == 0) return getEmptyString();
- const ssize_t bytes = utf16_to_utf8_length(in, len);
- if (bytes < 0) {
+ // Allow for closing '\0'
+ const ssize_t resultStrLen = utf16_to_utf8_length(in, len) + 1;
+ if (resultStrLen < 1) {
return getEmptyString();
}
- SharedBuffer* buf = SharedBuffer::alloc(bytes+1);
+ SharedBuffer* buf = SharedBuffer::alloc(resultStrLen);
ALOG_ASSERT(buf, "Unable to allocate shared buffer");
if (!buf) {
return getEmptyString();
}
- char* str = (char*)buf->data();
- utf16_to_utf8(in, len, str);
- return str;
+ char* resultStr = (char*)buf->data();
+ utf16_to_utf8(in, len, resultStr, resultStrLen);
+ return resultStr;
}
static char* allocFromUTF32(const char32_t* in, size_t len)
@@ -126,21 +127,21 @@
return getEmptyString();
}
- const ssize_t bytes = utf32_to_utf8_length(in, len);
- if (bytes < 0) {
+ const ssize_t resultStrLen = utf32_to_utf8_length(in, len) + 1;
+ if (resultStrLen < 1) {
return getEmptyString();
}
- SharedBuffer* buf = SharedBuffer::alloc(bytes+1);
+ SharedBuffer* buf = SharedBuffer::alloc(resultStrLen);
ALOG_ASSERT(buf, "Unable to allocate shared buffer");
if (!buf) {
return getEmptyString();
}
- char* str = (char*) buf->data();
- utf32_to_utf8(in, len, str);
+ char* resultStr = (char*) buf->data();
+ utf32_to_utf8(in, len, resultStr, resultStrLen);
- return str;
+ return resultStr;
}
// ---------------------------------------------------------------------------
diff --git a/libutils/Unicode.cpp b/libutils/Unicode.cpp
index f1f8bc9..ba084f6 100644
--- a/libutils/Unicode.cpp
+++ b/libutils/Unicode.cpp
@@ -14,6 +14,7 @@
* limitations under the License.
*/
+#include <log/log.h>
#include <utils/Unicode.h>
#include <stddef.h>
@@ -182,7 +183,7 @@
return ret;
}
-void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst)
+void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len)
{
if (src == NULL || src_len == 0 || dst == NULL) {
return;
@@ -193,9 +194,12 @@
char *cur = dst;
while (cur_utf32 < end_utf32) {
size_t len = utf32_codepoint_utf8_length(*cur_utf32);
+ LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len);
utf32_codepoint_to_utf8((uint8_t *)cur, *cur_utf32++, len);
cur += len;
+ dst_len -= len;
}
+ LOG_ALWAYS_FATAL_IF(dst_len < 1, "dst_len < 1: %zu < 1", dst_len);
*cur = '\0';
}
@@ -348,7 +352,7 @@
: 0);
}
-void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst)
+void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len)
{
if (src == NULL || src_len == 0 || dst == NULL) {
return;
@@ -369,9 +373,12 @@
utf32 = (char32_t) *cur_utf16++;
}
const size_t len = utf32_codepoint_utf8_length(utf32);
+ LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len);
utf32_codepoint_to_utf8((uint8_t*)cur, utf32, len);
cur += len;
+ dst_len -= len;
}
+ LOG_ALWAYS_FATAL_IF(dst_len < 1, "%zu < 1", dst_len);
*cur = '\0';
}
@@ -432,10 +439,10 @@
const char16_t* const end = src + src_len;
while (src < end) {
if ((*src & 0xFC00) == 0xD800 && (src + 1) < end
- && (*++src & 0xFC00) == 0xDC00) {
+ && (*(src + 1) & 0xFC00) == 0xDC00) {
// surrogate pairs are always 4 bytes.
ret += 4;
- src++;
+ src += 2;
} else {
ret += utf32_codepoint_utf8_length((char32_t) *src++);
}
diff --git a/libutils/tests/String8_test.cpp b/libutils/tests/String8_test.cpp
index 01e64f6..3947a5f 100644
--- a/libutils/tests/String8_test.cpp
+++ b/libutils/tests/String8_test.cpp
@@ -17,6 +17,7 @@
#define LOG_TAG "String8_test"
#include <utils/Log.h>
#include <utils/String8.h>
+#include <utils/String16.h>
#include <gtest/gtest.h>
@@ -77,4 +78,22 @@
EXPECT_EQ(NO_MEMORY, String8("").setTo(in, SIZE_MAX));
}
+// http://b/29250543
+TEST_F(String8Test, CorrectInvalidSurrogate) {
+ // d841d8 is an invalid start for a surrogate pair. Make sure this is handled by ignoring the
+ // first character in the pair and handling the rest correctly.
+ String16 string16(u"\xd841\xd841\xdc41\x0000");
+ String8 string8(string16);
+
+ EXPECT_EQ(4U, string8.length());
+}
+
+TEST_F(String8Test, CheckUtf32Conversion) {
+ // Since bound checks were added, check the conversion can be done without fatal errors.
+ // The utf8 lengths of these are chars are 1 + 2 + 3 + 4 = 10.
+ const char32_t string32[] = U"\x0000007f\x000007ff\x0000911\x0010fffe";
+ String8 string8(string32);
+ EXPECT_EQ(10U, string8.length());
+}
+
}