From b8837396df7cc422a3a76ee6cebed1182c030b65 Mon Sep 17 00:00:00 2001 From: Yi-Yo Chiang Date: Thu, 11 Nov 2021 22:17:35 +0800 Subject: [PATCH 1/2] Reland "Replace strtok_r() with C++-style android::base::Tokenize()" android::base::Tokenize() is like android::base::Split() but ignores empty tokens. Think strtok_r() and strsep(). C++-ify parsing code by replacing strtok_r() with Tokenize(), which results in more concise and readable code. Bug: 204056804 Test: atest CtsFsMgrTestCases (cherry picked from commit 3c1b581fd5f67487c67ccf3f7ab6fd3beb0b29ef) Change-Id: Icce8c1b5ad074421052f68fa138d90adb85cca27 --- fs_mgr/fs_mgr_fstab.cpp | 49 +++++++++++++---------------------------- 1 file changed, 15 insertions(+), 34 deletions(-) diff --git a/fs_mgr/fs_mgr_fstab.cpp b/fs_mgr/fs_mgr_fstab.cpp index 9b5152910..bee086ffd 100644 --- a/fs_mgr/fs_mgr_fstab.cpp +++ b/fs_mgr/fs_mgr_fstab.cpp @@ -531,11 +531,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; - const char *delim = " \t"; - char *save_ptr, *p; + char* p; Fstab fstab; while ((len = getline(&line, &alloc_len, fstab_file)) != -1) { @@ -553,42 +553,23 @@ 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(); + goto err; + } + FstabEntry entry; + auto it = fields.begin(); - 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; - } - - ParseMountFlags(p, &entry); + entry.blk_device = std::move(*it++); + entry.mount_point = std::move(*it++); + entry.fs_type = std::move(*it++); + ParseMountFlags(std::move(*it++), &entry); // For /proc/mounts, ignore everything after mnt_freq and mnt_passno - 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)) { + if (!proc_mounts && !ParseFsMgrFlags(std::move(*it++), &entry)) { LERROR << "Error parsing fs_mgr_flags"; goto err; } From 97f2fdff68cd647202610f6c973a2f80f7ef1238 Mon Sep 17 00:00:00 2001 From: Yi-Yo Chiang Date: Fri, 12 Nov 2021 19:19:29 +0800 Subject: [PATCH 2/2] Reland "Add ParseFstabFromString(), remove ReadFstabFromFp()" ReadFstabFromFp() have two callers right now, ReadFstabFromFile() and ReadFstabFromDt(). ReadFstabFromFile() opens a FILE* and pass it to ReadFstabFromFp(), and ReadFstabFromDt() wraps a std::string::c_str() buffer in a FILE* adaptor with fmemopen(). There's no need for such adaptor, just change ReadFstabFromFp() to accept std::string and we're good. Bug: 206740783 Bug: 204056804 Test: atest CtsFsMgrTestCases Test: m libfstab_fuzzer Change-Id: I3f56a83ec5baf7b0d97a618a2c2bb6e31b67b5d9 --- 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, 25 insertions(+), 52 deletions(-) diff --git a/fs_mgr/fs_mgr_fstab.cpp b/fs_mgr/fs_mgr_fstab.cpp index bee086ffd..d68b7e80e 100644 --- a/fs_mgr/fs_mgr_fstab.cpp +++ b/fs_mgr/fs_mgr_fstab.cpp @@ -530,34 +530,23 @@ bool EraseFstabEntry(Fstab* fstab, const std::string& mount_point) { } // namespace -bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out) { +bool ParseFstabFromString(const std::string& fstab_str, 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; - 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; - + 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; + } + if (fields.size() < expected_fields) { LERROR << "Error parsing fstab: expected " << expected_fields << " fields, got " << fields.size(); - goto err; + return false; } FstabEntry entry; @@ -571,7 +560,7 @@ bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out) { // 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"; - goto err; + return false; } if (entry.fs_mgr_flags.logical) { @@ -583,21 +572,17 @@ bool ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out) { if (fstab.empty()) { LERROR << "No entries found in fstab"; - goto err; + return false; } /* 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"; - goto err; + return false; } - free(line); + *fstab_out = std::move(fstab); return true; - -err: - free(line); - return false; } void TransformFstabForDsu(Fstab* fstab, const std::string& dsu_slot, @@ -696,16 +681,16 @@ void EnableMandatoryFlags(Fstab* fstab) { } bool ReadFstabFromFile(const std::string& path, Fstab* fstab_out) { - auto fstab_file = std::unique_ptr{fopen(path.c_str(), "re"), fclose}; - if (!fstab_file) { - PERROR << __FUNCTION__ << "(): cannot open file: '" << path << "'"; + const bool is_proc_mounts = (path == "/proc/mounts"); + + std::string fstab_str; + if (!android::base::ReadFileToString(path, &fstab_str, /* follow_symlinks = */ true)) { + PERROR << __FUNCTION__ << "(): failed to read file: '" << path << "'"; return false; } - bool is_proc_mounts = path == "/proc/mounts"; - Fstab fstab; - if (!ReadFstabFromFp(fstab_file.get(), is_proc_mounts, &fstab)) { + if (!ParseFstabFromString(fstab_str, is_proc_mounts, &fstab)) { LERROR << __FUNCTION__ << "(): failed to load fstab from : '" << path << "'"; return false; } @@ -752,15 +737,7 @@ bool ReadFstabFromDt(Fstab* fstab, bool verbose) { return false; } - 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 (!ParseFstabFromString(fstab_buf, /* proc_mounts = */ 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 1fddbf82d..6a8a19176 100644 --- a/fs_mgr/fuzz/fs_mgr_fstab_fuzzer.cpp +++ b/fs_mgr/fuzz/fs_mgr_fstab_fuzzer.cpp @@ -19,12 +19,8 @@ #include extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { - std::unique_ptr fstab_file( - fmemopen(static_cast(const_cast(data)), size, "r"), fclose); - if (fstab_file == nullptr) { - return 0; - } + std::string make_fstab_str(reinterpret_cast(data), size); android::fs_mgr::Fstab fstab; - android::fs_mgr::ReadFstabFromFp(fstab_file.get(), /* proc_mounts= */ false, &fstab); + android::fs_mgr::ParseFstabFromString(make_fstab_str, /* 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 d9c326d42..054300e8a 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 ReadFstabFromFp(FILE* fstab_file, bool proc_mounts, Fstab* fstab_out); +// Exported for testability. Regular users should use ReadFstabFromFile(). +bool ParseFstabFromString(const std::string& fstab_str, bool proc_mounts, Fstab* fstab_out); bool ReadFstabFromFile(const std::string& path, Fstab* fstab); bool ReadFstabFromDt(Fstab* fstab, bool verbose = true);