From 9dabc9ef320ce3651dda4db4881cdb49686c4f52 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 2 Oct 2023 17:02:05 -0700 Subject: [PATCH] Change source to be a unique_ptr. The current code keeps a pointer to a local variable which doesn't work too well. Change this to a unique_ptr and allocate the source object that will be used instead. Test: All unit tests pass. Test: fastboot -w flashall on a mokey which crashed without this change. Change-Id: Ief5437374181e514928c45dd540b42898901a137 --- fastboot/fastboot.cpp | 20 ++++++++++---------- fastboot/fastboot.h | 3 ++- fastboot/task.cpp | 4 ++-- fastboot/task_test.cpp | 18 +++++++----------- 4 files changed, 21 insertions(+), 24 deletions(-) diff --git a/fastboot/fastboot.cpp b/fastboot/fastboot.cpp index 3ce814179..b12e584d6 100644 --- a/fastboot/fastboot.cpp +++ b/fastboot/fastboot.cpp @@ -1525,7 +1525,7 @@ void do_flash(const char* pname, const char* fname, const bool apply_vbmeta, fb->ResizePartition(pname, std::to_string(buf.image_size)); } std::string flash_pname = repack_ramdisk(pname, &buf, fp->fb); - flash_buf(fp->source, flash_pname, &buf, apply_vbmeta); + flash_buf(fp->source.get(), flash_pname, &buf, apply_vbmeta); } // Sets slot_override as the active slot. If slot_override is blank, @@ -1679,7 +1679,7 @@ bool AddResizeTasks(const FlashingPlan* fp, std::vector>* } for (size_t i = 0; i < tasks->size(); i++) { if (auto flash_task = tasks->at(i)->AsFlashTask()) { - if (FlashTask::IsDynamicParitition(fp->source, flash_task)) { + if (FlashTask::IsDynamicParitition(fp->source.get(), flash_task)) { if (!loc) { loc = i; } @@ -1810,7 +1810,8 @@ std::vector> FlashAllTool::CollectTasks() { if (fp_->exclude_dynamic_partitions) { auto is_non_static_flash_task = [&](const auto& task) -> bool { if (auto flash_task = task->AsFlashTask()) { - if (!should_flash_in_userspace(fp_->source, flash_task->GetPartitionAndSlot())) { + if (!should_flash_in_userspace(fp_->source.get(), + flash_task->GetPartitionAndSlot())) { return false; } } @@ -1884,9 +1885,10 @@ std::vector> FlashAllTool::CollectTasksFromImageList() { // On these devices, secondary slots must be flashed as physical // partitions (otherwise they would not mount on first boot). To enforce // this, we delete any logical partitions for the "other" slot. - if (is_retrofit_device(fp_->source)) { + if (is_retrofit_device(fp_->source.get())) { std::string partition_name = image->part_name + "_" + slot; - if (image->IsSecondary() && should_flash_in_userspace(fp_->source, partition_name)) { + if (image->IsSecondary() && + should_flash_in_userspace(fp_->source.get(), partition_name)) { tasks.emplace_back(std::make_unique(fp_, partition_name)); } } @@ -1949,8 +1951,7 @@ static void do_update(const char* filename, FlashingPlan* fp) { if (error != 0) { die("failed to open zip file '%s': %s", filename, ErrorCodeString(error)); } - ZipImageSource zp = ZipImageSource(zip); - fp->source = &zp; + fp->source.reset(new ZipImageSource(zip)); FlashAllTool tool(fp); tool.Flash(); @@ -1971,8 +1972,7 @@ unique_fd LocalImageSource::OpenFile(const std::string& name) const { } static void do_flashall(FlashingPlan* fp) { - LocalImageSource s = LocalImageSource(); - fp->source = &s; + fp->source.reset(new LocalImageSource()); FlashAllTool tool(fp); tool.Flash(); } @@ -2089,7 +2089,7 @@ void fb_perform_format(const std::string& partition, int skip_if_not_supported, die("Cannot read image: %s", strerror(errno)); } - flash_buf(fp->source, partition, &buf, is_vbmeta_partition(partition)); + flash_buf(fp->source.get(), partition, &buf, is_vbmeta_partition(partition)); return; failed: diff --git a/fastboot/fastboot.h b/fastboot/fastboot.h index 4859cebc2..cd7bc2d98 100644 --- a/fastboot/fastboot.h +++ b/fastboot/fastboot.h @@ -28,6 +28,7 @@ #pragma once #include +#include #include #include "fastboot_driver_interface.h" #include "filesystem.h" @@ -89,7 +90,7 @@ struct FlashingPlan { // If the image uses the default slot, or the user specified "all", then // the paired string will be empty. If the image requests a specific slot // (for example, system_other) it is specified instead. - ImageSource* source; + std::unique_ptr source; bool wants_wipe = false; bool skip_reboot = false; bool wants_set_active = false; diff --git a/fastboot/task.cpp b/fastboot/task.cpp index 4b2b9e316..25c5a6e6b 100644 --- a/fastboot/task.cpp +++ b/fastboot/task.cpp @@ -41,7 +41,7 @@ bool FlashTask::IsDynamicParitition(const ImageSource* source, const FlashTask* void FlashTask::Run() { auto flash = [&](const std::string& partition) { - if (should_flash_in_userspace(fp_->source, partition) && !is_userspace_fastboot() && + if (should_flash_in_userspace(fp_->source.get(), partition) && !is_userspace_fastboot() && !fp_->force_flash) { die("The partition you are trying to flash is dynamic, and " "should be flashed via fastbootd. Please run:\n" @@ -174,7 +174,7 @@ std::unique_ptr OptimizedFlashSuperTask::Initialize( LOG(VERBOSE) << "Cannot optimize flashing super for all slots"; return nullptr; } - if (!CanOptimize(fp->source, tasks)) { + if (!CanOptimize(fp->source.get(), tasks)) { return nullptr; } diff --git a/fastboot/task_test.cpp b/fastboot/task_test.cpp index 1e25b6f63..9cde1a81e 100644 --- a/fastboot/task_test.cpp +++ b/fastboot/task_test.cpp @@ -192,8 +192,7 @@ TEST_F(ParseTest, CorrectTaskLists) { GTEST_SKIP(); } - LocalImageSource s; - fp->source = &s; + fp->source.reset(new LocalImageSource); fp->sparse_limit = std::numeric_limits::max(); fastboot::MockFastbootDriver fb; @@ -239,8 +238,7 @@ TEST_F(ParseTest, IsDynamicParitiontest) { GTEST_SKIP(); } - LocalImageSource s; - fp->source = &s; + fp->source.reset(new LocalImageSource); fastboot::MockFastbootDriver fb; fp->fb = &fb; @@ -260,7 +258,7 @@ TEST_F(ParseTest, IsDynamicParitiontest) { ParseFastbootInfoLine(fp.get(), android::base::Tokenize(test.first, " ")); auto flash_task = task->AsFlashTask(); ASSERT_FALSE(flash_task == nullptr); - ASSERT_EQ(FlashTask::IsDynamicParitition(fp->source, flash_task), test.second); + ASSERT_EQ(FlashTask::IsDynamicParitition(fp->source.get(), flash_task), test.second); } } @@ -269,8 +267,7 @@ TEST_F(ParseTest, CanOptimizeTest) { GTEST_SKIP(); } - LocalImageSource s; - fp->source = &s; + fp->source.reset(new LocalImageSource); fp->sparse_limit = std::numeric_limits::max(); fastboot::MockFastbootDriver fb; @@ -301,7 +298,7 @@ TEST_F(ParseTest, CanOptimizeTest) { for (auto& test : patternmatchtest) { std::vector> tasks = ParseFastbootInfo(fp.get(), test.first); tasks.erase(std::remove_if(tasks.begin(), tasks.end(), remove_if_callback), tasks.end()); - ASSERT_EQ(OptimizedFlashSuperTask::CanOptimize(fp->source, tasks), test.second); + ASSERT_EQ(OptimizedFlashSuperTask::CanOptimize(fp->source.get(), tasks), test.second); } } @@ -312,8 +309,7 @@ TEST_F(ParseTest, OptimizedFlashSuperPatternMatchTest) { GTEST_SKIP(); } - LocalImageSource s; - fp->source = &s; + fp->source.reset(new LocalImageSource); fp->sparse_limit = std::numeric_limits::max(); fastboot::MockFastbootDriver fb; @@ -362,7 +358,7 @@ TEST_F(ParseTest, OptimizedFlashSuperPatternMatchTest) { contains_optimized_task = true; } if (auto flash_task = task->AsFlashTask()) { - if (FlashTask::IsDynamicParitition(fp->source, flash_task)) { + if (FlashTask::IsDynamicParitition(fp->source.get(), flash_task)) { return false; } }