diff --git a/adb/adb_utils.h b/adb/adb_utils.h index cf420678b..cd6717d06 100644 --- a/adb/adb_utils.h +++ b/adb/adb_utils.h @@ -20,6 +20,7 @@ #include #include +#include void close_stdin(); @@ -50,6 +51,14 @@ bool set_file_block_mode(int fd, bool block); 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; + class ScopedFd { public: ScopedFd() { diff --git a/adb/shell_service.cpp b/adb/shell_service.cpp index f58af9f49..f98394d0d 100644 --- a/adb/shell_service.cpp +++ b/adb/shell_service.cpp @@ -160,9 +160,14 @@ class Subprocess { 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, + std::string* _Nonnull error); + private: // Opens the file at |pts_name|. int OpenPtyChildFd(const char* pts_name, ScopedFd* error_sfd); @@ -391,14 +396,19 @@ bool Subprocess::ForkAndExec(std::string* error) { } } - if (!adb_thread_create(ThreadHandler, this)) { + D("subprocess parent: completed"); + return true; +} + +bool Subprocess::StartThread(std::unique_ptr 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 @@ void Subprocess::ThreadHandler(void* userdata) { 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 @@ int StartSubprocess(const char* name, const char* terminal_type, protocol == SubprocessProtocol::kNone ? "none" : "shell", terminal_type, name); - Subprocess* subprocess = new Subprocess(name, terminal_type, type, protocol); + auto subprocess = std::make_unique(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 @@ int StartSubprocess(const char* name, const char* terminal_type, 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 81d201e65..75dcc8631 100644 --- a/adb/sysdeps.h +++ b/adb/sysdeps.h @@ -29,8 +29,9 @@ #include #include -// Include this before open/unlink are defined as macros below. +// Include this before open/close/unlink are defined as macros below. #include +#include #include /* diff --git a/base/include/android-base/errors.h b/base/include/android-base/errors.h index ca621fa8b..04c299c86 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 @@ -43,4 +43,4 @@ std::string SystemErrorCodeToString(int error_code); } // 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 5342d9878..aa18ea796 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 #include @@ -46,4 +46,4 @@ bool RemoveFileIfExists(const std::string& path, std::string* err = nullptr); } // 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 c82f8581c..d3f9d0cdc 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 @@ class ScopedLogSeverity { } // 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 b1ce7c6ba..913a9a03e 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 // for size_t #include // for TEMP_FAILURE_RETRY @@ -185,4 +185,4 @@ void UNUSED(const T&...) { } 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 882582f89..3a2f8fad6 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 @@ static inline void put_unaligned(T* address, T v) { } // 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 0543795e1..ed75e2d48 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 #include @@ -70,4 +70,4 @@ bool ParseInt(const char* s, T* out, } // 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 2de5ac93d..b4ac0256b 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 @@ -35,4 +35,4 @@ bool ParseNetAddress(const std::string& address, std::string* host, int* port, } // 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 d68af8713..cf666abe0 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 #include @@ -53,4 +53,4 @@ void StringAppendV(std::string* dst, const char* format, va_list ap) } // 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 20da144a4..69781cd40 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 #include @@ -65,4 +65,4 @@ bool EndsWith(const std::string& s, const char* suffix); } // 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 3f6872c3b..4ea3c8e48 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 @@ -48,4 +48,4 @@ class TemporaryDir { 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 90979df07..24221024c 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 d3b27cae3..869e60f58 100644 --- a/base/include/android-base/unique_fd.h +++ b/base/include/android-base/unique_fd.h @@ -19,39 +19,54 @@ #include -#include +// 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 +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 @@ class unique_fd final { } 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 @@ class unique_fd final { 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; + } // 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 3b0ed0af8..2d5a6f6d7 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 @@ -84,4 +84,4 @@ using ::unlink; } // 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 b7dd9ba8c..01c2cc105 100644 --- a/bootstat/boot_event_record_store_test.cpp +++ b/bootstat/boot_event_record_store_test.cpp @@ -41,7 +41,7 @@ namespace { // 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 @@ bool CreateEmptyBootEventRecord(const std::string& record_path, int32_t value) { // 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 7cca7c15d..57b232128 100644 --- a/libmemunreachable/ProcessMappings.cpp +++ b/libmemunreachable/ProcessMappings.cpp @@ -30,11 +30,10 @@ bool ProcessMappings(pid_t pid, allocator::vector& 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 e8a8392cc..9155c2928 100644 --- a/libmemunreachable/ThreadCapture.cpp +++ b/libmemunreachable/ThreadCapture.cpp @@ -108,12 +108,11 @@ bool ThreadCaptureImpl::ListThreads(TidList& tids) { 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 @@ bool ThreadCaptureImpl::ListThreads(TidList& tids) { 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 cefe94e6a..41ed84ee0 100644 --- a/libmemunreachable/tests/ThreadCapture_test.cpp +++ b/libmemunreachable/tests/ThreadCapture_test.cpp @@ -28,8 +28,6 @@ #include -#include - #include "Allocator.h" #include "ScopedDisableMalloc.h" #include "ScopedPipe.h"