adb: fix fd double close, Subprocess lifetime issue.
This commit fixes two somewhat related issues in shell_service.
- The fd returned by StartSubprocess is owned by a unique_fd
contained in the Subprocess object, but also gets closed by the
caller. Resolve this by duping the returned file descriptor.
- A Subprocess object can be destroyed immediately after its initial
construction in StartSubprocess if we're sufficiently unlucky.
Split up the fork/exec and "start management thread" steps, so that
we can safely do everything we need to do on the Subprocess before
handing it over to the thread that'll eventually destroy it.
Also includes squashed patches from AOSP master that allow for use of
unique_fd inside adb.
Bug: http://b/29254462
Change-Id: Id9cf0b7e7a7293bee7176919edc758597691c636
(cherry picked from commit c0e6e40cc916747a0a22c2538874348cda0ef607)
(cherry picked from commit 54c72aaccc45edf4832cfdc5053d9ae6acc9bcdf)
(cherry picked from commit 2c5d1d7cd914ec8ebf76c8a59d0889ebf5b538cd)
(cherry picked from commit 2a7b86337f7b149887588e4df532272abe3e931c)
(cherry picked from commit 13ea01db451b3993d175110a3336a58482be883d)
(cherry picked from commit 344778da411ebb47966961f3a70ca0848425194f)
diff --git a/adb/adb_utils.h b/adb/adb_utils.h
index cf42067..cd6717d 100644
--- a/adb/adb_utils.h
+++ b/adb/adb_utils.h
@@ -20,6 +20,7 @@
#include <string>
#include <android-base/macros.h>
+#include <android-base/unique_fd.h>
void close_stdin();
@@ -50,6 +51,14 @@
extern int adb_close(int fd);
// Helper to automatically close an FD when it goes out of scope.
+struct AdbCloser {
+ static void Close(int fd) {
+ adb_close(fd);
+ }
+};
+
+using unique_fd = android::base::unique_fd_impl<AdbCloser>;
+
class ScopedFd {
public:
ScopedFd() {
diff --git a/adb/shell_service.cpp b/adb/shell_service.cpp
index f58af9f..f98394d 100644
--- a/adb/shell_service.cpp
+++ b/adb/shell_service.cpp
@@ -160,9 +160,14 @@
pid_t pid() const { return pid_; }
// Sets up FDs, forks a subprocess, starts the subprocess manager thread,
- // and exec's the child. Returns false on failure.
+ // and exec's the child. Returns false and sets error on failure.
bool ForkAndExec(std::string* _Nonnull error);
+ // Start the subprocess manager thread. Consumes the subprocess, regardless of success.
+ // Returns false and sets error on failure.
+ static bool StartThread(std::unique_ptr<Subprocess> subprocess,
+ std::string* _Nonnull error);
+
private:
// Opens the file at |pts_name|.
int OpenPtyChildFd(const char* pts_name, ScopedFd* error_sfd);
@@ -391,14 +396,19 @@
}
}
- if (!adb_thread_create(ThreadHandler, this)) {
+ D("subprocess parent: completed");
+ return true;
+}
+
+bool Subprocess::StartThread(std::unique_ptr<Subprocess> subprocess, std::string* error) {
+ Subprocess* raw = subprocess.release();
+ if (!adb_thread_create(ThreadHandler, raw)) {
*error =
android::base::StringPrintf("failed to create subprocess thread: %s", strerror(errno));
- kill(pid_, SIGKILL);
+ kill(raw->pid_, SIGKILL);
return false;
}
- D("subprocess parent: completed");
return true;
}
@@ -442,6 +452,7 @@
adb_thread_setname(android::base::StringPrintf(
"shell srvc %d", subprocess->local_socket_fd()));
+ D("passing data streams for PID %d", subprocess->pid());
subprocess->PassDataStreams();
D("deleting Subprocess for PID %d", subprocess->pid());
@@ -738,7 +749,7 @@
protocol == SubprocessProtocol::kNone ? "none" : "shell",
terminal_type, name);
- Subprocess* subprocess = new Subprocess(name, terminal_type, type, protocol);
+ auto subprocess = std::make_unique<Subprocess>(name, terminal_type, type, protocol);
if (!subprocess) {
LOG(ERROR) << "failed to allocate new subprocess";
return ReportError(protocol, "failed to allocate new subprocess");
@@ -747,11 +758,17 @@
std::string error;
if (!subprocess->ForkAndExec(&error)) {
LOG(ERROR) << "failed to start subprocess: " << error;
- delete subprocess;
return ReportError(protocol, error);
}
- D("subprocess creation successful: local_socket_fd=%d, pid=%d",
- subprocess->local_socket_fd(), subprocess->pid());
- return subprocess->local_socket_fd();
+ unique_fd local_socket(dup(subprocess->local_socket_fd()));
+ D("subprocess creation successful: local_socket_fd=%d, pid=%d", local_socket.get(),
+ subprocess->pid());
+
+ if (!Subprocess::StartThread(std::move(subprocess), &error)) {
+ LOG(ERROR) << "failed to start subprocess management thread: " << error;
+ return ReportError(protocol, error);
+ }
+
+ return local_socket.release();
}
diff --git a/adb/sysdeps.h b/adb/sysdeps.h
index 81d201e..75dcc86 100644
--- a/adb/sysdeps.h
+++ b/adb/sysdeps.h
@@ -29,8 +29,9 @@
#include <string>
#include <vector>
-// Include this before open/unlink are defined as macros below.
+// Include this before open/close/unlink are defined as macros below.
#include <android-base/errors.h>
+#include <android-base/unique_fd.h>
#include <android-base/utf8.h>
/*
diff --git a/base/include/android-base/errors.h b/base/include/android-base/errors.h
index ca621fa..04c299c 100644
--- a/base/include/android-base/errors.h
+++ b/base/include/android-base/errors.h
@@ -27,8 +27,8 @@
// special handling to get the error string. Refer to Microsoft documentation
// to determine which error code to check for each function.
-#ifndef BASE_ERRORS_H
-#define BASE_ERRORS_H
+#ifndef ANDROID_BASE_ERRORS_H
+#define ANDROID_BASE_ERRORS_H
#include <string>
@@ -43,4 +43,4 @@
} // namespace base
} // namespace android
-#endif // BASE_ERRORS_H
+#endif // ANDROID_BASE_ERRORS_H
diff --git a/base/include/android-base/file.h b/base/include/android-base/file.h
index 5342d98..aa18ea7 100644
--- a/base/include/android-base/file.h
+++ b/base/include/android-base/file.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef BASE_FILE_H
-#define BASE_FILE_H
+#ifndef ANDROID_BASE_FILE_H
+#define ANDROID_BASE_FILE_H
#include <sys/stat.h>
#include <string>
@@ -46,4 +46,4 @@
} // namespace base
} // namespace android
-#endif // BASE_FILE_H
+#endif // ANDROID_BASE_FILE_H
diff --git a/base/include/android-base/logging.h b/base/include/android-base/logging.h
index c82f858..d3f9d0c 100644
--- a/base/include/android-base/logging.h
+++ b/base/include/android-base/logging.h
@@ -13,8 +13,9 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-#ifndef BASE_LOGGING_H
-#define BASE_LOGGING_H
+
+#ifndef ANDROID_BASE_LOGGING_H
+#define ANDROID_BASE_LOGGING_H
// NOTE: For Windows, you must include logging.h after windows.h to allow the
// following code to suppress the evil ERROR macro:
@@ -334,4 +335,4 @@
} // namespace base
} // namespace android
-#endif // BASE_LOGGING_H
+#endif // ANDROID_BASE_LOGGING_H
diff --git a/base/include/android-base/macros.h b/base/include/android-base/macros.h
index b1ce7c6..913a9a0 100644
--- a/base/include/android-base/macros.h
+++ b/base/include/android-base/macros.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef UTILS_MACROS_H
-#define UTILS_MACROS_H
+#ifndef ANDROID_BASE_MACROS_H
+#define ANDROID_BASE_MACROS_H
#include <stddef.h> // for size_t
#include <unistd.h> // for TEMP_FAILURE_RETRY
@@ -185,4 +185,4 @@
} while (0)
#endif
-#endif // UTILS_MACROS_H
+#endif // ANDROID_BASE_MACROS_H
diff --git a/base/include/android-base/memory.h b/base/include/android-base/memory.h
index 882582f..3a2f8fa 100644
--- a/base/include/android-base/memory.h
+++ b/base/include/android-base/memory.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef BASE_MEMORY_H
-#define BASE_MEMORY_H
+#ifndef ANDROID_BASE_MEMORY_H
+#define ANDROID_BASE_MEMORY_H
namespace android {
namespace base {
@@ -44,4 +44,4 @@
} // namespace base
} // namespace android
-#endif // BASE_MEMORY_H
+#endif // ANDROID_BASE_MEMORY_H
diff --git a/base/include/android-base/parseint.h b/base/include/android-base/parseint.h
index 0543795..ed75e2d 100644
--- a/base/include/android-base/parseint.h
+++ b/base/include/android-base/parseint.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef BASE_PARSEINT_H
-#define BASE_PARSEINT_H
+#ifndef ANDROID_BASE_PARSEINT_H
+#define ANDROID_BASE_PARSEINT_H
#include <errno.h>
#include <stdlib.h>
@@ -70,4 +70,4 @@
} // namespace base
} // namespace android
-#endif // BASE_PARSEINT_H
+#endif // ANDROID_BASE_PARSEINT_H
diff --git a/base/include/android-base/parsenetaddress.h b/base/include/android-base/parsenetaddress.h
index 2de5ac9..b4ac025 100644
--- a/base/include/android-base/parsenetaddress.h
+++ b/base/include/android-base/parsenetaddress.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef BASE_PARSENETADDRESS_H
-#define BASE_PARSENETADDRESS_H
+#ifndef ANDROID_BASE_PARSENETADDRESS_H
+#define ANDROID_BASE_PARSENETADDRESS_H
#include <string>
@@ -35,4 +35,4 @@
} // namespace base
} // namespace android
-#endif // BASE_PARSENETADDRESS_H
+#endif // ANDROID_BASE_PARSENETADDRESS_H
diff --git a/base/include/android-base/stringprintf.h b/base/include/android-base/stringprintf.h
index d68af87..cf666ab 100644
--- a/base/include/android-base/stringprintf.h
+++ b/base/include/android-base/stringprintf.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef BASE_STRINGPRINTF_H
-#define BASE_STRINGPRINTF_H
+#ifndef ANDROID_BASE_STRINGPRINTF_H
+#define ANDROID_BASE_STRINGPRINTF_H
#include <stdarg.h>
#include <string>
@@ -53,4 +53,4 @@
} // namespace base
} // namespace android
-#endif // BASE_STRINGPRINTF_H
+#endif // ANDROID_BASE_STRINGPRINTF_H
diff --git a/base/include/android-base/strings.h b/base/include/android-base/strings.h
index 20da144..69781cd 100644
--- a/base/include/android-base/strings.h
+++ b/base/include/android-base/strings.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef BASE_STRINGS_H
-#define BASE_STRINGS_H
+#ifndef ANDROID_BASE_STRINGS_H
+#define ANDROID_BASE_STRINGS_H
#include <sstream>
#include <string>
@@ -65,4 +65,4 @@
} // namespace base
} // namespace android
-#endif // BASE_STRINGS_H
+#endif // ANDROID_BASE_STRINGS_H
diff --git a/base/include/android-base/test_utils.h b/base/include/android-base/test_utils.h
index 3f6872c..4ea3c8e 100644
--- a/base/include/android-base/test_utils.h
+++ b/base/include/android-base/test_utils.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef TEST_UTILS_H
-#define TEST_UTILS_H
+#ifndef ANDROID_BASE_TEST_UTILS_H
+#define ANDROID_BASE_TEST_UTILS_H
#include <string>
@@ -48,4 +48,4 @@
DISALLOW_COPY_AND_ASSIGN(TemporaryDir);
};
-#endif // TEST_UTILS_H
+#endif // ANDROID_BASE_TEST_UTILS_H
diff --git a/base/include/android-base/thread_annotations.h b/base/include/android-base/thread_annotations.h
index 90979df..2422102 100644
--- a/base/include/android-base/thread_annotations.h
+++ b/base/include/android-base/thread_annotations.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef UTILS_THREAD_ANNOTATIONS_H
-#define UTILS_THREAD_ANNOTATIONS_H
+#ifndef ANDROID_BASE_THREAD_ANNOTATIONS_H
+#define ANDROID_BASE_THREAD_ANNOTATIONS_H
#if defined(__SUPPORT_TS_ANNOTATION__) || defined(__clang__)
#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))
@@ -80,4 +80,4 @@
#define NO_THREAD_SAFETY_ANALYSIS \
THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)
-#endif // UTILS_THREAD_ANNOTATIONS_H
+#endif // ANDROID_BASE_THREAD_ANNOTATIONS_H
diff --git a/base/include/android-base/unique_fd.h b/base/include/android-base/unique_fd.h
index d3b27ca..869e60f 100644
--- a/base/include/android-base/unique_fd.h
+++ b/base/include/android-base/unique_fd.h
@@ -19,39 +19,54 @@
#include <unistd.h>
-#include <android-base/macros.h>
+// DO NOT INCLUDE OTHER LIBBASE HEADERS!
+// This file gets used in libbinder, and libbinder is used everywhere.
+// Including other headers from libbase frequently results in inclusion of
+// android-base/macros.h, which causes macro collisions.
-/* Container for a file descriptor that automatically closes the descriptor as
- * it goes out of scope.
- *
- * unique_fd ufd(open("/some/path", "r"));
- *
- * if (ufd.get() < 0) // invalid descriptor
- * return error;
- *
- * // Do something useful
- *
- * return 0; // descriptor is closed here
- */
+// Container for a file descriptor that automatically closes the descriptor as
+// it goes out of scope.
+//
+// unique_fd ufd(open("/some/path", "r"));
+// if (ufd.get() == -1) return error;
+//
+// // Do something useful, possibly including 'return'.
+//
+// return 0; // Descriptor is closed for you.
+//
+// unique_fd is also known as ScopedFd/ScopedFD/scoped_fd; mentioned here to help
+// you find this class if you're searching for one of those names.
namespace android {
namespace base {
-class unique_fd final {
+struct DefaultCloser {
+ static void Close(int fd) {
+ // Even if close(2) fails with EINTR, the fd will have been closed.
+ // Using TEMP_FAILURE_RETRY will either lead to EBADF or closing someone
+ // else's fd.
+ // http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
+ ::close(fd);
+ }
+};
+
+template <typename Closer>
+class unique_fd_impl final {
public:
- unique_fd() : value_(-1) {}
+ unique_fd_impl() : value_(-1) {}
- explicit unique_fd(int value) : value_(value) {}
- ~unique_fd() { clear(); }
+ explicit unique_fd_impl(int value) : value_(value) {}
+ ~unique_fd_impl() { clear(); }
- unique_fd(unique_fd&& other) : value_(other.release()) {}
- unique_fd& operator = (unique_fd&& s) {
+ unique_fd_impl(unique_fd_impl&& other) : value_(other.release()) {}
+ unique_fd_impl& operator=(unique_fd_impl&& s) {
reset(s.release());
return *this;
}
void reset(int new_value) {
- if (value_ >= 0)
- close(value_);
+ if (value_ != -1) {
+ Closer::Close(value_);
+ }
value_ = new_value;
}
@@ -60,8 +75,9 @@
}
int get() const { return value_; }
+ operator int() const { return get(); }
- int release() {
+ int release() __attribute__((warn_unused_result)) {
int ret = value_;
value_ = -1;
return ret;
@@ -70,10 +86,13 @@
private:
int value_;
- DISALLOW_COPY_AND_ASSIGN(unique_fd);
+ unique_fd_impl(const unique_fd_impl&);
+ void operator=(const unique_fd_impl&);
};
+using unique_fd = unique_fd_impl<DefaultCloser>;
+
} // namespace base
} // namespace android
-#endif // ANDROID_BASE_UNIQUE_FD_H
+#endif // ANDROID_BASE_UNIQUE_FD_H
diff --git a/base/include/android-base/utf8.h b/base/include/android-base/utf8.h
index 3b0ed0a..2d5a6f6 100755
--- a/base/include/android-base/utf8.h
+++ b/base/include/android-base/utf8.h
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-#ifndef BASE_UTF8_H
-#define BASE_UTF8_H
+#ifndef ANDROID_BASE_UTF8_H
+#define ANDROID_BASE_UTF8_H
#ifdef _WIN32
#include <string>
@@ -84,4 +84,4 @@
} // namespace base
} // namespace android
-#endif // BASE_UTF8_H
+#endif // ANDROID_BASE_UTF8_H
diff --git a/bootstat/boot_event_record_store_test.cpp b/bootstat/boot_event_record_store_test.cpp
index b7dd9ba..01c2cc1 100644
--- a/bootstat/boot_event_record_store_test.cpp
+++ b/bootstat/boot_event_record_store_test.cpp
@@ -41,7 +41,7 @@
// the value of the record.
bool CreateEmptyBootEventRecord(const std::string& record_path, int32_t value) {
android::base::unique_fd record_fd(creat(record_path.c_str(), S_IRUSR | S_IWUSR));
- if (record_fd.get() == -1) {
+ if (record_fd == -1) {
return false;
}
@@ -49,7 +49,7 @@
// ensure the validity of the file mtime value, i.e., to check that the record
// file mtime values are not changed once set.
// TODO(jhawkins): Remove this block.
- if (!android::base::WriteStringToFd(std::to_string(value), record_fd.get())) {
+ if (!android::base::WriteStringToFd(std::to_string(value), record_fd)) {
return false;
}
diff --git a/libmemunreachable/ProcessMappings.cpp b/libmemunreachable/ProcessMappings.cpp
index 7cca7c1..57b2321 100644
--- a/libmemunreachable/ProcessMappings.cpp
+++ b/libmemunreachable/ProcessMappings.cpp
@@ -30,11 +30,10 @@
bool ProcessMappings(pid_t pid, allocator::vector<Mapping>& mappings) {
char map_buffer[1024];
snprintf(map_buffer, sizeof(map_buffer), "/proc/%d/maps", pid);
- int fd = open(map_buffer, O_RDONLY);
- if (fd < 0) {
+ android::base::unique_fd fd(open(map_buffer, O_RDONLY));
+ if (fd == -1) {
return false;
}
- android::base::unique_fd fd_guard{fd};
LineBuffer line_buf(fd, map_buffer, sizeof(map_buffer));
char* line;
diff --git a/libmemunreachable/ThreadCapture.cpp b/libmemunreachable/ThreadCapture.cpp
index e8a8392..9155c29 100644
--- a/libmemunreachable/ThreadCapture.cpp
+++ b/libmemunreachable/ThreadCapture.cpp
@@ -108,12 +108,11 @@
strlcat(path, pid_str, sizeof(path));
strlcat(path, "/task", sizeof(path));
- int fd = open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY);
- if (fd < 0) {
+ android::base::unique_fd fd(open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY));
+ if (fd == -1) {
ALOGE("failed to open %s: %s", path, strerror(errno));
return false;
}
- android::base::unique_fd fd_guard{fd};
struct linux_dirent64 {
uint64_t d_ino;
@@ -125,7 +124,7 @@
char dirent_buf[4096];
ssize_t nread;
do {
- nread = syscall(SYS_getdents64, fd, dirent_buf, sizeof(dirent_buf));
+ nread = syscall(SYS_getdents64, fd.get(), dirent_buf, sizeof(dirent_buf));
if (nread < 0) {
ALOGE("failed to get directory entries from %s: %s", path, strerror(errno));
return false;
diff --git a/libmemunreachable/tests/ThreadCapture_test.cpp b/libmemunreachable/tests/ThreadCapture_test.cpp
index cefe94e..41ed84e 100644
--- a/libmemunreachable/tests/ThreadCapture_test.cpp
+++ b/libmemunreachable/tests/ThreadCapture_test.cpp
@@ -28,8 +28,6 @@
#include <gtest/gtest.h>
-#include <android-base/unique_fd.h>
-
#include "Allocator.h"
#include "ScopedDisableMalloc.h"
#include "ScopedPipe.h"
