Commit: ffc2ed986654651d3e72502fb33fd2b2a64ef0ca - system-core (git)

system/core

Commit MetaInfo

• Révision: ffc2ed986654651d3e72502fb33fd2b2a64ef0ca (tree)
• l'heure: 2016-09-15 04:23:21
• Auteur: Jaap Jan Meijer <jjmeijer88@gmai...>
• Commiter: Jaap Jan Meijer

Message de Log

Merge remote-tracking branch 'cm/cm-13.0' into cm-13.0-x86

Conflicts:
adb/sockets.cpp

Change Summary

• modified: adb/Android.mk
• modified: adb/mutex_list.h
• modified: adb/sockets.cpp
• add: adb/sysdeps/mutex.h
• modified: debuggerd/backtrace.cpp
• modified: debuggerd/debuggerd.cpp
• modified: debuggerd/tombstone.cpp
• modified: debuggerd/utility.cpp
• modified: debuggerd/utility.h
• modified: include/utils/Unicode.h
• modified: libutils/String8.cpp
• modified: libutils/Unicode.cpp
• modified: libutils/tests/String8_test.cpp

Modification

--- a/adb/Android.mk
+++ b/adb/Android.mk
@@ -200,7 +200,10 @@ endif
 # will violate ODR
 LOCAL_SHARED_LIBRARIES :=
 
+# Don't build the host adb on Windows (this branch should only be used for security updates.)
+ifneq ($(HOST_OS),windows)
 include $(BUILD_HOST_EXECUTABLE)
+endif
 
 $(call dist-for-goals,dist_files sdk,$(LOCAL_BUILT_MODULE))
 

--- a/adb/mutex_list.h
+++ b/adb/mutex_list.h
@@ -6,7 +6,6 @@
 #ifndef ADB_MUTEX
 #error ADB_MUTEX not defined when including this file
 #endif
-ADB_MUTEX(socket_list_lock)
 ADB_MUTEX(transport_lock)
 ADB_MUTEX(fdevent_lock)
 #if ADB_HOST

--- a/adb/sockets.cpp
+++ b/adb/sockets.cpp
@@ -25,18 +25,25 @@
 #include <string.h>
 #include <unistd.h>
 
+#include <algorithm>
+#include <mutex>
+#include <string>
+#include <vector>
+
 #if !ADB_HOST
 #include "cutils/properties.h"
 #endif
 
 #include "adb.h"
 #include "adb_io.h"
+#include "sysdeps/mutex.h"
 #include "transport.h"
 
-ADB_MUTEX_DEFINE( socket_list_lock );
-
-static void local_socket_close_locked(asocket *s);
+#if !defined(__BIONIC__)
+using std::recursive_mutex;
+#endif
 
+static recursive_mutex& local_socket_list_lock = *new recursive_mutex();
 static unsigned local_socket_next_id = 1;
 
 static asocket local_socket_list = {
@@ -53,13 +60,16 @@ static asocket local_socket_closing_list = {
     .prev = &local_socket_closing_list,
 };
 
-static asocket *
-find_socket_in_list(asocket *list, unsigned local_id, unsigned peer_id)
+// Parse the global list of sockets to find one with id |local_id|.
+// If |peer_id| is not 0, also check that it is connected to a peer
+// with id |peer_id|. Returns an asocket handle on success, NULL on failure.
+asocket *find_local_socket(unsigned local_id, unsigned peer_id)
 {
     asocket *s;
     asocket *result = NULL;
 
-    for (s = list->next; s != list; s = s->next) {
+    std::lock_guard<recursive_mutex> lock(local_socket_list_lock);
+    for (s = local_socket_list.next; s != &local_socket_list; s = s->next) {
         if (s->id != local_id)
             continue;
         if (peer_id == 0 || (s->peer && s->peer->id == peer_id)) {
@@ -71,20 +81,6 @@ find_socket_in_list(asocket *list, unsigned local_id, unsigned peer_id)
     return result;
 }
 
-
-// Parse the global list of sockets to find one with id |local_id|.
-// If |peer_id| is not 0, also check that it is connected to a peer
-// with id |peer_id|. Returns an asocket handle on success, NULL on failure.
-asocket *find_local_socket(unsigned local_id, unsigned peer_id)
-{
-    asocket *result = NULL;
-
-    adb_mutex_lock(&socket_list_lock);
-    result = find_socket_in_list(&local_socket_list, local_id, peer_id);
-    adb_mutex_unlock(&socket_list_lock);
-    return result;
-}
-
 static void
 insert_local_socket(asocket*  s, asocket*  list)
 {
@@ -94,20 +90,17 @@ insert_local_socket(asocket*  s, asocket*  list)
     s->next->prev = s;
 }
 
-
-void install_local_socket(asocket *s)
-{
-    adb_mutex_lock(&socket_list_lock);
+void install_local_socket(asocket* s) {
+    std::lock_guard<recursive_mutex> lock(local_socket_list_lock);
 
     s->id = local_socket_next_id++;
 
     // Socket ids should never be 0.
-    if (local_socket_next_id == 0)
-      local_socket_next_id = 1;
+    if (local_socket_next_id == 0) {
+        fatal("local socket id overflow");
+    }
 
     insert_local_socket(s, &local_socket_list);
-
-    adb_mutex_unlock(&socket_list_lock);
 }
 
 void remove_socket(asocket *s)
@@ -129,30 +122,14 @@ void remove_socket(asocket *s)
 void close_all_sockets(atransport *t)
 {
     asocket *s;
-
-    D("close all sockets of transport %p\n", t);
-        /* this is a little gross, but since s->close() *will* modify
-        ** the list out from under you, your options are limited.
-        */
-    adb_mutex_lock(&socket_list_lock);
+    std::lock_guard<recursive_mutex> lock(local_socket_list_lock);
 restart:
-    for(s = local_socket_list.next; s != &local_socket_list; s = s->next){
-        if(s->transport == t || (s->peer && s->peer->transport == t)) {
-            // set force_close flag since transport is going to be removed.
-            // we need ensure the socket is closed after we return.
-            s->force_close = 1;
-            // avoid race condition with pending fdevent
-            if (s->running) {
-                // unlock to give a chance to close socket after running
-                adb_mutex_unlock(&socket_list_lock);
-                adb_sleep_ms(10); // sleep to relax cpu
-                adb_mutex_lock(&socket_list_lock);
-            } else
-                local_socket_close_locked(s);
+    for (s = local_socket_list.next; s != &local_socket_list; s = s->next) {
+        if (s->transport == t || (s->peer && s->peer->transport == t)) {
+            s->close(s);
             goto restart;
         }
     }
-    adb_mutex_unlock(&socket_list_lock);
 }
 
 static int local_socket_enqueue(asocket *s, apacket *p)
@@ -215,23 +192,6 @@ static void local_socket_ready(asocket *s)
     fdevent_add(&s->fde, FDE_READ);
 }
 
-static void local_socket_close(asocket *s)
-{
-    unsigned local_id = s->id;
-    unsigned peer_id = s->peer ? s->peer->id : 0;
-    asocket *sk;
-
-    adb_mutex_lock(&socket_list_lock);
-    // we may race with close_all_sockets (called by input-thread),
-    // so need to check if socket already destoried.
-    sk = find_socket_in_list(&local_socket_list, local_id, peer_id);
-    if (!sk)
-        sk = find_socket_in_list(&local_socket_closing_list, local_id, peer_id);
-    if (sk)
-        local_socket_close_locked(s);
-    adb_mutex_unlock(&socket_list_lock);
-}
-
 // be sure to hold the socket list lock when calling this
 static void local_socket_destroy(asocket  *s)
 {
@@ -260,27 +220,21 @@ static void local_socket_destroy(asocket  *s)
     }
 }
 
-
-static void local_socket_close_locked(asocket *s)
-{
-    D("entered local_socket_close_locked. LS(%d) fd=%d\n", s->id, s->fd);
-    if(s->peer) {
-        D("LS(%d): closing peer. peer->id=%d peer->fd=%d\n",
-          s->id, s->peer->id, s->peer->fd);
+static void local_socket_close(asocket* s) {
+    D("entered local_socket_close. LS(%d) fd=%d", s->id, s->fd);
+    std::lock_guard<recursive_mutex> lock(local_socket_list_lock);
+    if (s->peer) {
+        D("LS(%d): closing peer. peer->id=%d peer->fd=%d", s->id, s->peer->id, s->peer->fd);
         /* Note: it's important to call shutdown before disconnecting from
          * the peer, this ensures that remote sockets can still get the id
          * of the local socket they're connected to, to send a CLOSE()
          * protocol event. */
-        if (s->peer->shutdown)
-          s->peer->shutdown(s->peer);
-        s->peer->peer = 0;
-        // tweak to avoid deadlock
-        if (s->peer->close == local_socket_close) {
-            local_socket_close_locked(s->peer);
-        } else {
-            s->peer->close(s->peer);
+        if (s->peer->shutdown) {
+            s->peer->shutdown(s->peer);
         }
-        s->peer = 0;
+        s->peer->peer = nullptr;
+        s->peer->close(s->peer);
+        s->peer = nullptr;
     }
 
         /* If we are already closing, or if there are no

--- /dev/null
+++ b/adb/sysdeps/mutex.h
@@ -0,0 +1,143 @@
+#pragma once
+
+/*
+ * 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.
+ */
+
+#if defined(_WIN32)
+
+#include <windows.h>
+
+#include <android-base/macros.h>
+
+#include "adb.h"
+
+// The prebuilt version of mingw we use doesn't support mutex or recursive_mutex.
+// Therefore, implement our own using the Windows primitives.
+// Put them directly into the std namespace, so that when they're actually available, the build
+// breaks until they're removed.
+
+#include <mutex>
+namespace std {
+
+// CRITICAL_SECTION is recursive, so just wrap it in a Mutex-compatible class.
+class recursive_mutex {
+  public:
+    recursive_mutex() {
+        InitializeCriticalSection(&mutex_);
+    }
+
+    ~recursive_mutex() {
+        DeleteCriticalSection(&mutex_);
+    }
+
+    void lock() {
+        EnterCriticalSection(&mutex_);
+    }
+
+    bool try_lock() {
+        return TryEnterCriticalSection(&mutex_);
+    }
+
+    void unlock() {
+        LeaveCriticalSection(&mutex_);
+    }
+
+  private:
+    CRITICAL_SECTION mutex_;
+
+    DISALLOW_COPY_AND_ASSIGN(recursive_mutex);
+};
+
+class mutex {
+  public:
+    mutex() {
+    }
+
+    ~mutex() {
+    }
+
+    void lock() {
+        mutex_.lock();
+        if (++lock_count_ != 1) {
+            fatal("non-recursive mutex locked reentrantly");
+        }
+    }
+
+    void unlock() {
+        if (--lock_count_ != 0) {
+            fatal("non-recursive mutex unlock resulted in unexpected lock count: %d", lock_count_);
+        }
+        mutex_.unlock();
+    }
+
+    bool try_lock() {
+        if (!mutex_.try_lock()) {
+            return false;
+        }
+
+        if (lock_count_ != 0) {
+            mutex_.unlock();
+            return false;
+        }
+
+        ++lock_count_;
+        return true;
+    }
+
+  private:
+    recursive_mutex mutex_;
+    size_t lock_count_ = 0;
+};
+
+}
+
+#elif defined(__BIONIC__)
+
+// On M, the recovery image uses parts of adb that depends on recursive_mutex, and uses libstdc++,
+// which lacks it.
+
+#include <pthread.h>
+#include <mutex>
+
+#include <base/macros.h>
+
+class recursive_mutex {
+  public:
+    recursive_mutex() {
+    }
+
+    ~recursive_mutex() {
+    }
+
+    void lock() {
+        pthread_mutex_lock(&mutex_);
+    }
+
+    bool try_lock() {
+        return pthread_mutex_trylock(&mutex_);
+    }
+
+    void unlock() {
+        pthread_mutex_unlock(&mutex_);
+    }
+
+  private:
+    pthread_mutex_t mutex_ = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+
+    DISALLOW_COPY_AND_ASSIGN(recursive_mutex);
+};
+
+#endif

--- a/debuggerd/backtrace.cpp
+++ b/debuggerd/backtrace.cpp
@@ -67,8 +67,8 @@ static void dump_process_footer(log_t* log, pid_t pid) {
   _LOG(log, logtype::BACKTRACE, "\n----- end %d -----\n", pid);
 }
 
-static void dump_thread(
-    log_t* log, pid_t tid, bool attached, bool* detach_failed, int* total_sleep_time_usec) {
+static void dump_thread(log_t* log, pid_t pid, pid_t tid, bool attached,
+                        bool* detach_failed, int* total_sleep_time_usec) {
   char path[PATH_MAX];
   char threadnamebuf[1024];
   char* threadname = NULL;
@@ -88,7 +88,7 @@ static void dump_thread(
 
   _LOG(log, logtype::BACKTRACE, "\n\"%s\" sysTid=%d\n", threadname ? threadname : "<unknown>", tid);
 
-  if (!attached && ptrace(PTRACE_ATTACH, tid, 0, 0) < 0) {
+  if (!attached && !ptrace_attach_thread(pid, tid)) {
     _LOG(log, logtype::BACKTRACE, "Could not attach to thread: %s\n", strerror(errno));
     return;
   }
@@ -117,7 +117,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
   log.amfd = amfd;
 
   dump_process_header(&log, pid);
-  dump_thread(&log, tid, true, detach_failed, total_sleep_time_usec);
+  dump_thread(&log, pid, tid, true, detach_failed, total_sleep_time_usec);
 
   char task_path[64];
   snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);
@@ -135,7 +135,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
         continue;
       }
 
-      dump_thread(&log, new_tid, false, detach_failed, total_sleep_time_usec);
+      dump_thread(&log, pid, new_tid, false, detach_failed, total_sleep_time_usec);
     }
     closedir(d);
   }

--- a/debuggerd/debuggerd.cpp
+++ b/debuggerd/debuggerd.cpp
@@ -308,15 +308,13 @@ static int read_request(int fd, debugger_request_t* out_request) {
 
   if (msg.action == DEBUGGER_ACTION_CRASH) {
     // Ensure that the tid reported by the crashing process is valid.
-    char buf[64];
-    struct stat s;
-    enable_etb_trace(cr);
-    snprintf(buf, sizeof buf, "/proc/%d/task/%d", out_request->pid, out_request->tid);
-    if (stat(buf, &s)) {
+    // 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);
+            out_request->tid, out_request->pid);
       return -1;
     }
+    enable_etb_trace(cr);
   } 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.
@@ -476,9 +474,32 @@ static void handle_request(int fd) {
     // ensure that it can run as soon as we call PTRACE_CONT below.
     // See details in bionic/libc/linker/debugger.c, in function
     // debugger_signal_handler().
-    if (ptrace(PTRACE_ATTACH, request.tid, 0, 0)) {
+    if (!ptrace_attach_thread(request.pid, request.tid)) {
       ALOGE("ptrace attach failed: %s\n", strerror(errno));
     } else {
+      // 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);
+        }
+      }
+
       bool detach_failed = false;
       bool tid_unresponsive = false;
       bool attach_gdb = should_attach_gdb(&request);

--- a/debuggerd/tombstone.cpp
+++ b/debuggerd/tombstone.cpp
@@ -450,7 +450,7 @@ static bool dump_sibling_thread_report(
     }
 
     // Skip this thread if cannot ptrace it
-    if (ptrace(PTRACE_ATTACH, new_tid, 0, 0) < 0) {
+    if (!ptrace_attach_thread(pid, new_tid)) {
       _LOG(log, logtype::ERROR, "ptrace attach to %d failed: %s\n", new_tid, strerror(errno));
       continue;
     }

--- a/debuggerd/utility.cpp
+++ b/debuggerd/utility.cpp
@@ -20,6 +20,7 @@
 
 #include <errno.h>
 #include <signal.h>
+#include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 #include <sys/ptrace.h>
@@ -207,3 +208,31 @@ void dump_memory(log_t* log, Backtrace* backtrace, uintptr_t addr, const char* f
     _LOG(log, logtype::MEMORY, "%s  %s\n", logline.c_str(), ascii.c_str());
   }
 }
+
+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;
+}
+
+// Attach to a thread, and verify that it's still a member of the given process
+bool ptrace_attach_thread(pid_t pid, pid_t tid) {
+  if (ptrace(PTRACE_ATTACH, tid, 0, 0) != 0) {
+    return false;
+  }
+
+  // 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;
+}

--- a/debuggerd/utility.h
+++ b/debuggerd/utility.h
@@ -79,4 +79,9 @@ int wait_for_sigstop(pid_t, int*, bool*);
 
 void dump_memory(log_t* log, Backtrace* backtrace, uintptr_t addr, const char* fmt, ...);
 
+bool pid_contains_tid(pid_t pid, pid_t tid);
+
+// Attach to a thread, and verify that it's still a member of the given process
+bool ptrace_attach_thread(pid_t pid, pid_t tid);
+
 #endif // _DEBUGGERD_UTILITY_H

--- a/include/utils/Unicode.h
+++ b/include/utils/Unicode.h
@@ -87,7 +87,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len);
  * "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".
@@ -109,7 +109,7 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len);
  * 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.

--- a/libutils/String8.cpp
+++ b/libutils/String8.cpp
@@ -102,20 +102,21 @@ static char* allocFromUTF16(const char16_t* in, size_t len)
 {
     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)
@@ -124,21 +125,21 @@ static char* allocFromUTF32(const char32_t* in, size_t len)
         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;
 }
 
 // ---------------------------------------------------------------------------

--- 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 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len)
     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 @@ void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst)
     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';
 }
 
@@ -324,7 +328,7 @@ int strzcmp16_h_n(const char16_t *s1H, size_t n1, const char16_t *s2N, size_t n2
            : 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;
@@ -345,9 +349,12 @@ void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst)
             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';
 }
 
@@ -408,10 +415,10 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len)
     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++);
         }

--- 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>
 
@@ -72,4 +73,22 @@ TEST_F(String8Test, OperatorPlusEquals) {
     EXPECT_STREQ(src3, " Verify me.");
 }
 
+// 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());
+}
+
 }