From 62291bfd5c5d431b409e4142c1882952b8fe6070 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Wed, 17 Nov 2021 15:51:01 +0000 Subject: [PATCH 1/2] Revert "Add ParseFstabFromString(), remove ReadFstabFromFp()" Revert submission 1890098 Reason for revert: Breaks tests, b/206740783 Reverted Changes: I71190c735:Add ParseFstabFromString(), remove ReadFstabFromFp... Ic1dd0eb97:Replace strtok_r() with C++-style android::base::T... Change-Id: I1ded0217670a9bf3f2485120ee0dddf3e854a6fb --- fs_mgr/fs_mgr_fstab.cpp | 65 +++++++++++++++++++---------- fs_mgr/fuzz/fs_mgr_fstab_fuzzer.cpp | 8 +++- fs_mgr/include_fstab/fstab/fstab.h | 4 +- 3 files changed, 51 insertions(+), 26 deletions(-) diff --git a/fs_mgr/fs_mgr_fstab.cpp b/fs_mgr/fs_mgr_fstab.cpp index d53dc54b5..0b083e52d 100644 --- a/fs_mgr/fs_mgr_fstab.cpp +++ b/fs_mgr/fs_mgr_fstab.cpp @@ -525,23 +525,34 @@ bool EraseFstabEntry(Fstab* fstab, const std::string& mount_point) { } // namespace -bool ParseFstabFromString(const std::string& fstab_str, bool proc_mounts, Fstab* fstab_out) { +bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out) { const int expected_fields = proc_mounts ? 4 : 5; - + ssize_t len; + size_t alloc_len = 0; + char *line = NULL; + char* p; Fstab fstab; - for (const auto& line : android::base::Split(fstab_str, "\n")) { - auto fields = android::base::Tokenize(line, " \t"); - - // Ignore empty lines and comments. - if (fields.empty() || android::base::StartsWith(fields.front(), '#')) { - continue; + while ((len = getline(&line, &alloc_len, fstab_file)) != -1) { + /* if the last character is a newline, shorten the string by 1 byte */ + if (line[len - 1] == '\n') { + line[len - 1] = '\0'; } + /* Skip any leading whitespace */ + p = line; + while (isspace(*p)) { + p++; + } + /* ignore comments or empty lines */ + if (*p == '#' || *p == '\0') + continue; + + auto fields = android::base::Tokenize(line, " \t"); if (fields.size() < expected_fields) { LERROR << "Error parsing fstab: expected " << expected_fields << " fields, got " << fields.size(); - return false; + goto err; } FstabEntry entry; @@ -555,7 +566,7 @@ bool ParseFstabFromString(const std::string& fstab_str, bool proc_mounts, Fstab* // For /proc/mounts, ignore everything after mnt_freq and mnt_passno if (!proc_mounts && !ParseFsMgrFlags(std::move(*it++), &entry)) { LERROR << "Error parsing fs_mgr_flags"; - return false; + goto err; } if (entry.fs_mgr_flags.logical) { @@ -567,17 +578,21 @@ bool ParseFstabFromString(const std::string& fstab_str, bool proc_mounts, Fstab* if (fstab.empty()) { LERROR << "No entries found in fstab"; - return false; + goto err; } /* If an A/B partition, modify block device to be the real block device */ if (!fs_mgr_update_for_slotselect(&fstab)) { LERROR << "Error updating for slotselect"; - return false; + goto err; } - + free(line); *fstab_out = std::move(fstab); return true; + +err: + free(line); + return false; } void TransformFstabForDsu(Fstab* fstab, const std::string& dsu_slot, @@ -676,18 +691,16 @@ void EnableMandatoryFlags(Fstab* fstab) { } bool ReadFstabFromFile(const std::string& path, Fstab* fstab_out) { - const bool is_proc_mounts = (path == "/proc/mounts"); - // /proc/mounts could be a symlink to /proc/self/mounts. - const bool follow_symlinks = is_proc_mounts; - - std::string fstab_str; - if (!android::base::ReadFileToString(path, &fstab_str, follow_symlinks)) { - PERROR << __FUNCTION__ << "(): failed to read file: '" << path << "'"; + auto fstab_file = std::unique_ptr{fopen(path.c_str(), "re"), fclose}; + if (!fstab_file) { + PERROR << __FUNCTION__ << "(): cannot open file: '" << path << "'"; return false; } + bool is_proc_mounts = path == "/proc/mounts"; + Fstab fstab; - if (!ParseFstabFromString(fstab_str, is_proc_mounts, &fstab)) { + if (!ReadFstabFromFp(fstab_file.get(), is_proc_mounts, &fstab)) { LERROR << __FUNCTION__ << "(): failed to load fstab from : '" << path << "'"; return false; } @@ -734,7 +747,15 @@ bool ReadFstabFromDt(Fstab* fstab, bool verbose) { return false; } - if (!ParseFstabFromString(fstab_buf, /* proc_mounts = */ false, fstab)) { + std::unique_ptr fstab_file( + fmemopen(static_cast(const_cast(fstab_buf.c_str())), + fstab_buf.length(), "r"), fclose); + if (!fstab_file) { + if (verbose) PERROR << __FUNCTION__ << "(): failed to create a file stream for fstab dt"; + return false; + } + + if (!ReadFstabFromFp(fstab_file.get(), false, fstab)) { if (verbose) { LERROR << __FUNCTION__ << "(): failed to load fstab from kernel:" << std::endl << fstab_buf; diff --git a/fs_mgr/fuzz/fs_mgr_fstab_fuzzer.cpp b/fs_mgr/fuzz/fs_mgr_fstab_fuzzer.cpp index 6a8a19176..1fddbf82d 100644 --- a/fs_mgr/fuzz/fs_mgr_fstab_fuzzer.cpp +++ b/fs_mgr/fuzz/fs_mgr_fstab_fuzzer.cpp @@ -19,8 +19,12 @@ #include extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { - std::string make_fstab_str(reinterpret_cast(data), size); + std::unique_ptr fstab_file( + fmemopen(static_cast(const_cast(data)), size, "r"), fclose); + if (fstab_file == nullptr) { + return 0; + } android::fs_mgr::Fstab fstab; - android::fs_mgr::ParseFstabFromString(make_fstab_str, /* proc_mounts = */ false, &fstab); + android::fs_mgr::ReadFstabFromFp(fstab_file.get(), /* proc_mounts= */ false, &fstab); return 0; } diff --git a/fs_mgr/include_fstab/fstab/fstab.h b/fs_mgr/include_fstab/fstab/fstab.h index 054300e8a..d9c326d42 100644 --- a/fs_mgr/include_fstab/fstab/fstab.h +++ b/fs_mgr/include_fstab/fstab/fstab.h @@ -94,8 +94,8 @@ struct FstabEntry { // Unless explicitly requested, a lookup on mount point should always return the 1st one. using Fstab = std::vector; -// Exported for testability. Regular users should use ReadFstabFromFile(). -bool ParseFstabFromString(const std::string& fstab_str, bool proc_mounts, Fstab* fstab_out); +// Exported for testability. Regular users should use ReadFstabFromfile(). +bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out); bool ReadFstabFromFile(const std::string& path, Fstab* fstab); bool ReadFstabFromDt(Fstab* fstab, bool verbose = true); From 867916e8b528952b1530360f995e7d4dc3ef3fc0 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Wed, 17 Nov 2021 15:51:01 +0000 Subject: [PATCH 2/2] Revert "Replace strtok_r() with C++-style android::base::Tokenize()" Revert submission 1890098 Reason for revert: Breaks tests, b/206740783 Reverted Changes: I71190c735:Add ParseFstabFromString(), remove ReadFstabFromFp... Ic1dd0eb97:Replace strtok_r() with C++-style android::base::T... Change-Id: I1eecdc43d504385b00caec17db626eb1d623c8ef --- fs_mgr/fs_mgr_fstab.cpp | 47 +++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/fs_mgr/fs_mgr_fstab.cpp b/fs_mgr/fs_mgr_fstab.cpp index 0b083e52d..159be6772 100644 --- a/fs_mgr/fs_mgr_fstab.cpp +++ b/fs_mgr/fs_mgr_fstab.cpp @@ -526,11 +526,11 @@ bool EraseFstabEntry(Fstab* fstab, const std::string& mount_point) { } // namespace bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out) { - const int expected_fields = proc_mounts ? 4 : 5; ssize_t len; size_t alloc_len = 0; char *line = NULL; - char* p; + const char *delim = " \t"; + char *save_ptr, *p; Fstab fstab; while ((len = getline(&line, &alloc_len, fstab_file)) != -1) { @@ -548,23 +548,42 @@ bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out) { if (*p == '#' || *p == '\0') continue; - auto fields = android::base::Tokenize(line, " \t"); - if (fields.size() < expected_fields) { - LERROR << "Error parsing fstab: expected " << expected_fields << " fields, got " - << fields.size(); + FstabEntry entry; + + if (!(p = strtok_r(line, delim, &save_ptr))) { + LERROR << "Error parsing mount source"; + goto err; + } + entry.blk_device = p; + + if (!(p = strtok_r(NULL, delim, &save_ptr))) { + LERROR << "Error parsing mount_point"; + goto err; + } + entry.mount_point = p; + + if (!(p = strtok_r(NULL, delim, &save_ptr))) { + LERROR << "Error parsing fs_type"; + goto err; + } + entry.fs_type = p; + + if (!(p = strtok_r(NULL, delim, &save_ptr))) { + LERROR << "Error parsing mount_flags"; goto err; } - FstabEntry entry; - auto it = fields.begin(); - - entry.blk_device = std::move(*it++); - entry.mount_point = std::move(*it++); - entry.fs_type = std::move(*it++); - ParseMountFlags(std::move(*it++), &entry); + ParseMountFlags(p, &entry); // For /proc/mounts, ignore everything after mnt_freq and mnt_passno - if (!proc_mounts && !ParseFsMgrFlags(std::move(*it++), &entry)) { + if (proc_mounts) { + p += strlen(p); + } else if (!(p = strtok_r(NULL, delim, &save_ptr))) { + LERROR << "Error parsing fs_mgr_options"; + goto err; + } + + if (!ParseFsMgrFlags(p, &entry)) { LERROR << "Error parsing fs_mgr_flags"; goto err; }