From f274e78eeb2b1dcb951dddc89dd574a4909ac647 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Wed, 3 Oct 2018 13:13:41 -0700 Subject: [PATCH] fs_mgr/init: use unique_ptr + CLO_EXEC for setmntent()/fopen() We ran into an issue with an fd leaking due to missing both CLO_EXEC and fclose() in related code, so let's make sure we're safe here too. Test: boot Change-Id: Ief893c936859815c78fa6d7e06cb88ad34aadbac --- fs_mgr/fs_mgr.cpp | 26 +++++++--------- fs_mgr/fs_mgr_fstab.cpp | 6 ++-- fs_mgr/fs_mgr_verity.cpp | 59 ++++++++++++++---------------------- fs_mgr/tests/fs_mgr_test.cpp | 2 +- init/reboot.cpp | 2 +- 5 files changed, 37 insertions(+), 58 deletions(-) diff --git a/fs_mgr/fs_mgr.cpp b/fs_mgr/fs_mgr.cpp index f56043cd5..38ecc31d7 100644 --- a/fs_mgr/fs_mgr.cpp +++ b/fs_mgr/fs_mgr.cpp @@ -1235,29 +1235,25 @@ int fs_mgr_swapon_all(struct fstab *fstab) * on a system (all the memory comes from the same pool) so * we can assume the device number is 0. */ - FILE *zram_fp; - FILE *zram_mcs_fp; - if (fstab->recs[i].max_comp_streams >= 0) { - zram_mcs_fp = fopen(ZRAM_CONF_MCS, "r+"); - if (zram_mcs_fp == NULL) { - LERROR << "Unable to open zram conf comp device " - << ZRAM_CONF_MCS; - ret = -1; - continue; - } - fprintf(zram_mcs_fp, "%d\n", fstab->recs[i].max_comp_streams); - fclose(zram_mcs_fp); + auto zram_mcs_fp = std::unique_ptr{ + fopen(ZRAM_CONF_MCS, "re"), fclose}; + if (zram_mcs_fp == NULL) { + LERROR << "Unable to open zram conf comp device " << ZRAM_CONF_MCS; + ret = -1; + continue; + } + fprintf(zram_mcs_fp.get(), "%d\n", fstab->recs[i].max_comp_streams); } - zram_fp = fopen(ZRAM_CONF_DEV, "r+"); + auto zram_fp = + std::unique_ptr{fopen(ZRAM_CONF_DEV, "re+"), fclose}; if (zram_fp == NULL) { LERROR << "Unable to open zram conf device " << ZRAM_CONF_DEV; ret = -1; continue; } - fprintf(zram_fp, "%u\n", fstab->recs[i].zram_size); - fclose(zram_fp); + fprintf(zram_fp.get(), "%u\n", fstab->recs[i].zram_size); } if (fstab->recs[i].fs_mgr_flags & MF_WAIT && diff --git a/fs_mgr/fs_mgr_fstab.cpp b/fs_mgr/fs_mgr_fstab.cpp index f87a3b148..1585b4c88 100644 --- a/fs_mgr/fs_mgr_fstab.cpp +++ b/fs_mgr/fs_mgr_fstab.cpp @@ -730,21 +730,19 @@ static std::set extract_boot_devices(const fstab& fstab) { struct fstab *fs_mgr_read_fstab(const char *fstab_path) { - FILE *fstab_file; struct fstab *fstab; - fstab_file = fopen(fstab_path, "r"); + auto fstab_file = std::unique_ptr{fopen(fstab_path, "re"), fclose}; if (!fstab_file) { PERROR << __FUNCTION__<< "(): cannot open file: '" << fstab_path << "'"; return nullptr; } - fstab = fs_mgr_read_fstab_file(fstab_file, !strcmp("/proc/mounts", fstab_path)); + fstab = fs_mgr_read_fstab_file(fstab_file.get(), !strcmp("/proc/mounts", fstab_path)); if (!fstab) { LERROR << __FUNCTION__ << "(): failed to load fstab from : '" << fstab_path << "'"; } - fclose(fstab_file); return fstab; } diff --git a/fs_mgr/fs_mgr_verity.cpp b/fs_mgr/fs_mgr_verity.cpp index 5fb4ebba0..2727a6d32 100644 --- a/fs_mgr/fs_mgr_verity.cpp +++ b/fs_mgr/fs_mgr_verity.cpp @@ -89,24 +89,21 @@ static RSA *load_key(const char *path) { uint8_t key_data[ANDROID_PUBKEY_ENCODED_SIZE]; - FILE* f = fopen(path, "r"); + auto f = std::unique_ptr{fopen(path, "re"), fclose}; if (!f) { LERROR << "Can't open " << path; - return NULL; + return nullptr; } - if (!fread(key_data, sizeof(key_data), 1, f)) { + if (!fread(key_data, sizeof(key_data), 1, f.get())) { LERROR << "Could not read key!"; - fclose(f); - return NULL; + return nullptr; } - fclose(f); - - RSA* key = NULL; + RSA* key = nullptr; if (!android_pubkey_decode(key_data, sizeof(key_data), &key)) { LERROR << "Could not parse key!"; - return NULL; + return nullptr; } return key; @@ -368,7 +365,6 @@ static int metadata_add(FILE *fp, long start, const char *tag, static int metadata_find(const char *fname, const char *stag, unsigned int slength, off64_t *offset) { - FILE *fp = NULL; char tag[METADATA_TAG_MAX_LENGTH + 1]; int rc = -1; int n; @@ -380,75 +376,64 @@ static int metadata_find(const char *fname, const char *stag, return -1; } - fp = fopen(fname, "r+"); + auto fp = std::unique_ptr{fopen(fname, "re+"), fclose}; if (!fp) { PERROR << "Failed to open " << fname; - goto out; + return -1; } /* check magic */ - if (fseek(fp, start, SEEK_SET) < 0 || - fread(&magic, sizeof(magic), 1, fp) != 1) { + if (fseek(fp.get(), start, SEEK_SET) < 0 || fread(&magic, sizeof(magic), 1, fp.get()) != 1) { PERROR << "Failed to read magic from " << fname; - goto out; + return -1; } if (magic != METADATA_MAGIC) { magic = METADATA_MAGIC; - if (fseek(fp, start, SEEK_SET) < 0 || - fwrite(&magic, sizeof(magic), 1, fp) != 1) { + if (fseek(fp.get(), start, SEEK_SET) < 0 || + fwrite(&magic, sizeof(magic), 1, fp.get()) != 1) { PERROR << "Failed to write magic to " << fname; - goto out; + return -1; } - rc = metadata_add(fp, start + sizeof(magic), stag, slength, offset); + rc = metadata_add(fp.get(), start + sizeof(magic), stag, slength, offset); if (rc < 0) { PERROR << "Failed to add metadata to " << fname; } - goto out; + return rc; } start += sizeof(magic); while (1) { - n = fscanf(fp, "%" STRINGIFY(METADATA_TAG_MAX_LENGTH) "s %u\n", - tag, &length); + n = fscanf(fp.get(), "%" STRINGIFY(METADATA_TAG_MAX_LENGTH) "s %u\n", tag, &length); if (n == 2 && strcmp(tag, METADATA_EOD)) { /* found a tag */ - start = ftell(fp); + start = ftell(fp.get()); if (!strcmp(tag, stag) && length == slength) { *offset = start; - rc = 0; - goto out; + return 0; } start += length; - if (fseek(fp, length, SEEK_CUR) < 0) { + if (fseek(fp.get(), length, SEEK_CUR) < 0) { PERROR << "Failed to seek " << fname; - goto out; + return -1; } } else { - rc = metadata_add(fp, start, stag, slength, offset); + rc = metadata_add(fp.get(), start, stag, slength, offset); if (rc < 0) { PERROR << "Failed to write metadata to " << fname; } - goto out; + return rc; } - } - -out: - if (fp) { - fflush(fp); - fclose(fp); } - - return rc; } static int write_verity_state(const char *fname, off64_t offset, int32_t mode) diff --git a/fs_mgr/tests/fs_mgr_test.cpp b/fs_mgr/tests/fs_mgr_test.cpp index 8b1c55a6e..db01c1e3f 100644 --- a/fs_mgr/tests/fs_mgr_test.cpp +++ b/fs_mgr/tests/fs_mgr_test.cpp @@ -139,7 +139,7 @@ TEST(fs_mgr, fs_mgr_read_fstab_file_proc_mounts) { auto fstab = fs_mgr_read_fstab("/proc/mounts"); ASSERT_NE(fstab, nullptr); - std::unique_ptr mounts(setmntent("/proc/mounts", "r"), + std::unique_ptr mounts(setmntent("/proc/mounts", "re"), endmntent); ASSERT_NE(mounts, nullptr); diff --git a/init/reboot.cpp b/init/reboot.cpp index 2f88121b7..908c67ff0 100644 --- a/init/reboot.cpp +++ b/init/reboot.cpp @@ -162,7 +162,7 @@ static void LogShutdownTime(UmountStat stat, Timer* t) { */ static bool FindPartitionsToUmount(std::vector* blockDevPartitions, std::vector* emulatedPartitions, bool dump) { - std::unique_ptr fp(setmntent("/proc/mounts", "r"), endmntent); + std::unique_ptr fp(setmntent("/proc/mounts", "re"), endmntent); if (fp == nullptr) { PLOG(ERROR) << "Failed to open /proc/mounts"; return false;