From 358c2ec1dca24d48a503b441d38545ace021ea8e Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 28 Nov 2016 11:48:43 -0800 Subject: [PATCH] Remove ota_close(int) and ota_fclose(FILE*). We should always use unique_fd or unique_file to hold the FD or FILE* pointer when opening via ota_(f)open functions. This CL avoids accidentally closing raw FDs or FILE* pointers that are managed by unique_fd/unique_file. Test: recovery_component_test passes. Change-Id: If58eb8b5c5da507563f85efd5d56276472a1c957 --- applypatch/applypatch.cpp | 4 +- otafault/ota_io.cpp | 22 ++++-- otafault/ota_io.h | 16 ++-- updater/install.cpp | 156 +++++++++++++++++++------------------- 4 files changed, 101 insertions(+), 97 deletions(-) diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp index 0250407a..95389da6 100644 --- a/applypatch/applypatch.cpp +++ b/applypatch/applypatch.cpp @@ -66,7 +66,7 @@ int LoadFileContents(const char* filename, FileContents* file) { } std::vector data(file->st.st_size); - std::unique_ptr f(ota_fopen(filename, "rb"), ota_fclose); + unique_file f(ota_fopen(filename, "rb")); if (!f) { printf("failed to open \"%s\": %s\n", filename, strerror(errno)); return -1; @@ -118,7 +118,7 @@ static int LoadPartitionContents(const std::string& filename, FileContents* file std::sort(pairs.begin(), pairs.end()); const char* partition = pieces[1].c_str(); - std::unique_ptr dev(ota_fopen(partition, "rb"), ota_fclose); + unique_file dev(ota_fopen(partition, "rb")); if (!dev) { printf("failed to open emmc partition \"%s\": %s\n", partition, strerror(errno)); return -1; diff --git a/otafault/ota_io.cpp b/otafault/ota_io.cpp index 874cb1d5..f5b01136 100644 --- a/otafault/ota_io.cpp +++ b/otafault/ota_io.cpp @@ -70,23 +70,31 @@ FILE* ota_fopen(const char* path, const char* mode) { return fh; } -int ota_close(int fd) { +static int __ota_close(int fd) { // descriptors can be reused, so make sure not to leave them in the cache filename_cache.erase(fd); return close(fd); } -int ota_close(unique_fd& fd) { - return ota_close(fd.release()); +void OtaCloser::Close(int fd) { + __ota_close(fd); } -int ota_fclose(FILE* fh) { - filename_cache.erase((intptr_t)fh); +int ota_close(unique_fd& fd) { + return __ota_close(fd.release()); +} + +static int __ota_fclose(FILE* fh) { + filename_cache.erase(reinterpret_cast(fh)); return fclose(fh); } -int ota_fclose(std::unique_ptr& fh) { - return ota_fclose(fh.release()); +void OtaFcloser::operator()(FILE* f) { + __ota_fclose(f); +}; + +int ota_fclose(unique_file& fh) { + return __ota_fclose(fh.release()); } size_t ota_fread(void* ptr, size_t size, size_t nitems, FILE* stream) { diff --git a/otafault/ota_io.h b/otafault/ota_io.h index 45d17b1b..395b4230 100644 --- a/otafault/ota_io.h +++ b/otafault/ota_io.h @@ -40,10 +40,6 @@ int ota_open(const char* path, int oflags, mode_t mode); FILE* ota_fopen(const char* filename, const char* mode); -int ota_close(int fd); - -int ota_fclose(FILE* fh); - size_t ota_fread(void* ptr, size_t size, size_t nitems, FILE* stream); ssize_t ota_read(int fd, void* buf, size_t nbyte); @@ -55,15 +51,19 @@ ssize_t ota_write(int fd, const void* buf, size_t nbyte); int ota_fsync(int fd); struct OtaCloser { - static void Close(int fd) { - ota_close(fd); - } + static void Close(int); }; using unique_fd = android::base::unique_fd_impl; int ota_close(unique_fd& fd); -int ota_fclose(std::unique_ptr& fh); +struct OtaFcloser { + void operator()(FILE*); +}; + +using unique_file = std::unique_ptr; + +int ota_fclose(unique_file& fh); #endif diff --git a/updater/install.cpp b/updater/install.cpp index b9bc19e2..8db5c1fe 100644 --- a/updater/install.cpp +++ b/updater/install.cpp @@ -509,8 +509,8 @@ Value* PackageExtractFileFn(const char* name, State* state, int argc, Expr* argv return StringValue(""); } - int fd = TEMP_FAILURE_RETRY( - ota_open(dest_path.c_str(), O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR)); + unique_fd fd(TEMP_FAILURE_RETRY( + ota_open(dest_path.c_str(), O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR))); if (fd == -1) { printf("%s: can't open %s for write: %s\n", name, dest_path.c_str(), strerror(errno)); return StringValue(""); @@ -872,68 +872,67 @@ Value* GetPropFn(const char* name, State* state, int argc, Expr* argv[]) { // per line. # comment lines, blank lines, lines without '=' ignored), // and returns the value for 'key' (or "" if it isn't defined). Value* FileGetPropFn(const char* name, State* state, int argc, Expr* argv[]) { - if (argc != 2) { - return ErrorAbort(state, kArgsParsingFailure, "%s() expects 2 args, got %d", name, argc); + if (argc != 2) { + return ErrorAbort(state, kArgsParsingFailure, "%s() expects 2 args, got %d", name, argc); + } + + std::vector args; + if (!ReadArgs(state, 2, argv, &args)) { + return ErrorAbort(state, kArgsParsingFailure, "%s() Failed to parse the argument(s)", name); + } + const std::string& filename = args[0]; + const std::string& key = args[1]; + + struct stat st; + if (stat(filename.c_str(), &st) < 0) { + return ErrorAbort(state, kFileGetPropFailure, "%s: failed to stat \"%s\": %s", name, + filename.c_str(), strerror(errno)); + } + + constexpr off_t MAX_FILE_GETPROP_SIZE = 65536; + if (st.st_size > MAX_FILE_GETPROP_SIZE) { + return ErrorAbort(state, kFileGetPropFailure, "%s too large for %s (max %lld)", + filename.c_str(), name, static_cast(MAX_FILE_GETPROP_SIZE)); + } + + std::string buffer(st.st_size, '\0'); + unique_file f(ota_fopen(filename.c_str(), "rb")); + if (f == nullptr) { + return ErrorAbort(state, kFileOpenFailure, "%s: failed to open %s: %s", name, filename.c_str(), + strerror(errno)); + } + + if (ota_fread(&buffer[0], 1, st.st_size, f.get()) != static_cast(st.st_size)) { + ErrorAbort(state, kFreadFailure, "%s: failed to read %zu bytes from %s", name, + static_cast(st.st_size), filename.c_str()); + return nullptr; + } + + ota_fclose(f); + + std::vector lines = android::base::Split(buffer, "\n"); + for (size_t i = 0; i < lines.size(); i++) { + std::string line = android::base::Trim(lines[i]); + + // comment or blank line: skip to next line + if (line.empty() || line[0] == '#') { + continue; + } + size_t equal_pos = line.find('='); + if (equal_pos == std::string::npos) { + continue; } - std::vector args; - if (!ReadArgs(state, 2, argv, &args)) { - return ErrorAbort(state, kArgsParsingFailure, "%s() Failed to parse the argument(s)", name); - } - const std::string& filename = args[0]; - const std::string& key = args[1]; + // trim whitespace between key and '=' + std::string str = android::base::Trim(line.substr(0, equal_pos)); - struct stat st; - if (stat(filename.c_str(), &st) < 0) { - return ErrorAbort(state, kFileGetPropFailure, "%s: failed to stat \"%s\": %s", name, - filename.c_str(), strerror(errno)); - } + // not the key we're looking for + if (key != str) continue; - constexpr off_t MAX_FILE_GETPROP_SIZE = 65536; - if (st.st_size > MAX_FILE_GETPROP_SIZE) { - return ErrorAbort(state, kFileGetPropFailure, "%s too large for %s (max %lld)", - filename.c_str(), name, static_cast(MAX_FILE_GETPROP_SIZE)); - } + return StringValue(android::base::Trim(line.substr(equal_pos + 1))); + } - std::string buffer(st.st_size, '\0'); - FILE* f = ota_fopen(filename.c_str(), "rb"); - if (f == nullptr) { - return ErrorAbort(state, kFileOpenFailure, "%s: failed to open %s: %s", name, - filename.c_str(), strerror(errno)); - } - - if (ota_fread(&buffer[0], 1, st.st_size, f) != static_cast(st.st_size)) { - ErrorAbort(state, kFreadFailure, "%s: failed to read %zu bytes from %s", - name, static_cast(st.st_size), filename.c_str()); - ota_fclose(f); - return nullptr; - } - - ota_fclose(f); - - std::vector lines = android::base::Split(buffer, "\n"); - for (size_t i = 0; i < lines.size(); i++) { - std::string line = android::base::Trim(lines[i]); - - // comment or blank line: skip to next line - if (line.empty() || line[0] == '#') { - continue; - } - size_t equal_pos = line.find('='); - if (equal_pos == std::string::npos) { - continue; - } - - // trim whitespace between key and '=' - std::string str = android::base::Trim(line.substr(0, equal_pos)); - - // not the key we're looking for - if (key != str) continue; - - return StringValue(android::base::Trim(line.substr(equal_pos + 1))); - } - - return StringValue(""); + return StringValue(""); } // apply_patch_space(bytes) @@ -1292,29 +1291,26 @@ Value* GetStageFn(const char* name, State* state, int argc, Expr* argv[]) { } Value* WipeBlockDeviceFn(const char* name, State* state, int argc, Expr* argv[]) { - if (argc != 2) { - return ErrorAbort(state, kArgsParsingFailure, "%s() expects 2 args, got %d", name, argc); - } + if (argc != 2) { + return ErrorAbort(state, kArgsParsingFailure, "%s() expects 2 args, got %d", name, argc); + } - std::vector args; - if (!ReadArgs(state, 2, argv, &args)) { - return ErrorAbort(state, kArgsParsingFailure, "%s() Failed to parse the argument(s)", name); - } - const std::string& filename = args[0]; - const std::string& len_str = args[1]; + std::vector args; + if (!ReadArgs(state, 2, argv, &args)) { + return ErrorAbort(state, kArgsParsingFailure, "%s() Failed to parse the argument(s)", name); + } + const std::string& filename = args[0]; + const std::string& len_str = args[1]; - size_t len; - if (!android::base::ParseUint(len_str.c_str(), &len)) { - return nullptr; - } - int fd = ota_open(filename.c_str(), O_WRONLY, 0644); - // The wipe_block_device function in ext4_utils returns 0 on success and 1 - // for failure. - int status = wipe_block_device(fd, len); - - ota_close(fd); - - return StringValue((status == 0) ? "t" : ""); + size_t len; + if (!android::base::ParseUint(len_str.c_str(), &len)) { + return nullptr; + } + unique_fd fd(ota_open(filename.c_str(), O_WRONLY, 0644)); + // The wipe_block_device function in ext4_utils returns 0 on success and 1 + // for failure. + int status = wipe_block_device(fd, len); + return StringValue((status == 0) ? "t" : ""); } Value* EnableRebootFn(const char* name, State* state, int argc, Expr* argv[]) {