From dd6eefca303755264434454fddc0cc627e834577 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 19 Mar 2019 11:39:14 -0700 Subject: [PATCH 1/4] libcutils: android_get_control_file uses realpath. If the path to android_get_control_file is a symlink, the final sanity check will fail that the fd does not have the same path as the given file. We can't expect callers to readlink() because this would change the environment key. Instead, try to call realpath on both paths. Bug: 126233777 Test: lpdump Change-Id: I0df10d7dbe3e572b8335faad812e5cd80bff1733 --- libcutils/android_get_control_file.cpp | 35 +++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/libcutils/android_get_control_file.cpp b/libcutils/android_get_control_file.cpp index f9964d2b0..d5b0894b2 100644 --- a/libcutils/android_get_control_file.cpp +++ b/libcutils/android_get_control_file.cpp @@ -39,6 +39,11 @@ #include #include +#include + +#include +#include + #include "android_get_control_env.h" int __android_get_control_from_env(const char* prefix, const char* name) { @@ -72,26 +77,22 @@ int __android_get_control_from_env(const char* prefix, const char* name) { } int android_get_control_file(const char* path) { - int fd = __android_get_control_from_env(ANDROID_FILE_ENV_PREFIX, path); + std::string given_path; + if (!android::base::Realpath(path, &given_path)) return -1; + + // Try path, then realpath(path), as keys to get the fd from env. + auto fd = __android_get_control_from_env(ANDROID_FILE_ENV_PREFIX, path); + if (fd < 0) { + fd = __android_get_control_from_env(ANDROID_FILE_ENV_PREFIX, given_path.c_str()); + if (fd < 0) return fd; + } // Find file path from /proc and make sure it is correct - char *proc = NULL; - if (asprintf(&proc, "/proc/self/fd/%d", fd) < 0) return -1; - if (!proc) return -1; + auto proc = android::base::StringPrintf("/proc/self/fd/%d", fd); + std::string fd_path; + if (!android::base::Realpath(proc, &fd_path)) return -1; - size_t len = strlen(path); - // readlink() does not guarantee a nul byte, len+2 so we catch truncation. - char *buf = static_cast(calloc(1, len + 2)); - if (!buf) { - free(proc); - return -1; - } - ssize_t ret = TEMP_FAILURE_RETRY(readlink(proc, buf, len + 1)); - free(proc); - int cmp = (len != static_cast(ret)) || strcmp(buf, path); - free(buf); - if (ret < 0) return -1; - if (cmp != 0) return -1; + if (given_path != fd_path) return -1; // It is what we think it is return fd; From 567f1874fdd5765a6e20bc9f08b3264033267cb7 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 19 Mar 2019 14:38:48 -0700 Subject: [PATCH 2/4] init: expand prop in 'file' Allow having properties in 'file' option of a service. Test: boots (sanity) Test: lpdumpd Bug: 126233777 Change-Id: I55158b81e3829b393a9725fd8f09200690d0230f --- init/service.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/init/service.cpp b/init/service.cpp index cba42c4fd..6d08cb14a 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -756,6 +756,11 @@ Result Service::ParseFile(std::vector&& args) { if (args[2] != "r" && args[2] != "w" && args[2] != "rw") { return Error() << "file type must be 'r', 'w' or 'rw'"; } + std::string expanded; + if (!expand_props(args[1], &expanded)) { + return Error() << "Could not expand property in file path '" << args[1] << "'"; + } + args[1] = std::move(expanded); if ((args[1][0] != '/') || (args[1].find("../") != std::string::npos)) { return Error() << "file name must not be relative"; } From 26328e80b1a178c0b33ae0880878198b4212bd28 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 19 Mar 2019 12:08:48 -0700 Subject: [PATCH 3/4] liblp: Replace open with GetControlFileOrOpen ... so that fds from init can be used. Bug: 126233777 Test: unit tests Change-Id: Ife652e61305ef4fb6a02edfa765a91b5959d1b4b --- fs_mgr/liblp/Android.bp | 5 +++++ fs_mgr/liblp/images.cpp | 4 ++-- fs_mgr/liblp/partition_opener.cpp | 4 ++-- fs_mgr/liblp/utility.cpp | 18 ++++++++++++++++++ fs_mgr/liblp/utility.h | 3 +++ 5 files changed, 30 insertions(+), 4 deletions(-) diff --git a/fs_mgr/liblp/Android.bp b/fs_mgr/liblp/Android.bp index 355b7a1f1..70399942e 100644 --- a/fs_mgr/liblp/Android.bp +++ b/fs_mgr/liblp/Android.bp @@ -43,6 +43,11 @@ cc_library { windows: { enabled: true, }, + android: { + shared_libs: [ + "libcutils", + ], + }, }, export_include_dirs: ["include"], } diff --git a/fs_mgr/liblp/images.cpp b/fs_mgr/liblp/images.cpp index 5a498f975..56b535353 100644 --- a/fs_mgr/liblp/images.cpp +++ b/fs_mgr/liblp/images.cpp @@ -68,7 +68,7 @@ std::unique_ptr ReadFromImageBlob(const void* data, size_t bytes) { } std::unique_ptr ReadFromImageFile(const std::string& image_file) { - unique_fd fd(open(image_file.c_str(), O_RDONLY | O_CLOEXEC)); + unique_fd fd = GetControlFileOrOpen(image_file.c_str(), O_RDONLY | O_CLOEXEC); if (fd < 0) { PERROR << __PRETTY_FUNCTION__ << " open failed: " << image_file; return nullptr; @@ -408,7 +408,7 @@ bool SparseBuilder::CheckExtentOrdering() { } int SparseBuilder::OpenImageFile(const std::string& file) { - android::base::unique_fd source_fd(open(file.c_str(), O_RDONLY | O_CLOEXEC)); + android::base::unique_fd source_fd = GetControlFileOrOpen(file.c_str(), O_RDONLY | O_CLOEXEC); if (source_fd < 0) { PERROR << "open image file failed: " << file; return -1; diff --git a/fs_mgr/liblp/partition_opener.cpp b/fs_mgr/liblp/partition_opener.cpp index 898f24127..bb8ec9cb8 100644 --- a/fs_mgr/liblp/partition_opener.cpp +++ b/fs_mgr/liblp/partition_opener.cpp @@ -45,7 +45,7 @@ std::string GetPartitionAbsolutePath(const std::string& path) { bool GetBlockDeviceInfo(const std::string& block_device, BlockDeviceInfo* device_info) { #if defined(__linux__) - unique_fd fd(open(block_device.c_str(), O_RDONLY)); + unique_fd fd = GetControlFileOrOpen(block_device.c_str(), O_RDONLY); if (fd < 0) { PERROR << __PRETTY_FUNCTION__ << "open '" << block_device << "' failed"; return false; @@ -85,7 +85,7 @@ bool GetBlockDeviceInfo(const std::string& block_device, BlockDeviceInfo* device unique_fd PartitionOpener::Open(const std::string& partition_name, int flags) const { std::string path = GetPartitionAbsolutePath(partition_name); - return unique_fd{open(path.c_str(), flags | O_CLOEXEC)}; + return GetControlFileOrOpen(path.c_str(), flags | O_CLOEXEC); } bool PartitionOpener::GetInfo(const std::string& partition_name, BlockDeviceInfo* info) const { diff --git a/fs_mgr/liblp/utility.cpp b/fs_mgr/liblp/utility.cpp index ecf94a4c7..72a3c57ca 100644 --- a/fs_mgr/liblp/utility.cpp +++ b/fs_mgr/liblp/utility.cpp @@ -28,6 +28,10 @@ #include #include +#ifdef __ANDROID__ +#include +#endif + #include "utility.h" namespace android { @@ -171,5 +175,19 @@ bool SetBlockReadonly(int fd, bool readonly) { #endif } +base::unique_fd GetControlFileOrOpen(const char* path, int flags) { +#if defined(__ANDROID__) + int fd = android_get_control_file(path); + if (fd >= 0) { + int newfd = TEMP_FAILURE_RETRY(dup(fd)); + if (newfd >= 0) { + return base::unique_fd(newfd); + } + PERROR << "Cannot dup fd for already controlled file: " << path << ", reopening..."; + } +#endif + return base::unique_fd(open(path, flags)); +} + } // namespace fs_mgr } // namespace android diff --git a/fs_mgr/liblp/utility.h b/fs_mgr/liblp/utility.h index e8b2ca970..96f17174a 100644 --- a/fs_mgr/liblp/utility.h +++ b/fs_mgr/liblp/utility.h @@ -22,6 +22,7 @@ #include #include +#include #include "liblp/liblp.h" @@ -92,6 +93,8 @@ bool UpdatePartitionGroupName(LpMetadataPartitionGroup* group, const std::string // Call BLKROSET ioctl on fd so that fd is readonly / read-writable. bool SetBlockReadonly(int fd, bool readonly); +::android::base::unique_fd GetControlFileOrOpen(const char* path, int flags); + } // namespace fs_mgr } // namespace android From 5f4cb8a24060da1bd60f0580848fa2e94ade3f0a Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Thu, 21 Mar 2019 15:20:53 -0700 Subject: [PATCH 4/4] libbase: realpath is wrapped with TEMP_FAILURE_RETRY Although man page for realpath doesn't say so, the bionic implementation of realpath may exit with error code EINTR. In such cases, retry. Test: boots (sanity) Change-Id: Ic5feb7152374a81bd2f475ad74c4bc8c3afb3a20 --- base/file.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/base/file.cpp b/base/file.cpp index 2f4a51758..adc898489 100644 --- a/base/file.cpp +++ b/base/file.cpp @@ -385,7 +385,12 @@ bool Readlink(const std::string& path, std::string* result) { bool Realpath(const std::string& path, std::string* result) { result->clear(); - char* realpath_buf = realpath(path.c_str(), nullptr); + // realpath may exit with EINTR. Retry if so. + char* realpath_buf = nullptr; + do { + realpath_buf = realpath(path.c_str(), nullptr); + } while (realpath_buf == nullptr && errno == EINTR); + if (realpath_buf == nullptr) { return false; }