From 9a76d20e4fa6d4a3c29bb1aba662f0d839077782 Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Mon, 21 Jan 2019 15:56:48 +0000 Subject: Remove outfile option from dumpstate Remove the ability to write to any directory. Instead support only writing to its internal directory (currently /bugreports, which points to Shell app's directory), or a caller-specified file fd. We cannot expect all callers to supply an fd, because in the poweruser case where the bugreport is triggered with combo keys, the API is bypassed and dumpstate binary will be run. This should be a safe change since sepolicy should not allow dumpstate to write to arbitrary directories anyway. This keeps the API lean and keeps the user consent for sharing more focused. Note that the current callers all pass in /data/user_de/0/com.android.shell/files/bugreports/bugreport as the outfile argument already, which is the location /bugreports symlink points to, so it should work just as before. BUG:111441001 Test: adb bugreport Test: adb shell bugreport Test: interactive bugreport Test: adb shell /data/nativetest64/dumpstate_test/dumpstate_test Change-Id: Iae8593dc4745147b7bdae25738fcd69b3c20aaf0 --- cmds/dumpstate/DumpstateService.cpp | 6 ++- cmds/dumpstate/dumpstate.cpp | 69 ++++++++++----------------------- cmds/dumpstate/dumpstate.h | 19 +++++---- cmds/dumpstate/tests/dumpstate_test.cpp | 34 ++++------------ 4 files changed, 41 insertions(+), 87 deletions(-) diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index 1d5b738c04..ba9a96790d 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -144,6 +144,9 @@ binder::Status DumpstateService::startBugreport(int32_t /* calling_uid */, } status_t DumpstateService::dump(int fd, const Vector&) { + std::string destination = ds_.options_->bugreport_fd.get() != -1 + ? StringPrintf("[fd:%d]", ds_.options_->bugreport_fd.get()) + : ds_.bugreport_internal_dir_.c_str(); dprintf(fd, "id: %d\n", ds_.id_); dprintf(fd, "pid: %d\n", ds_.pid_); dprintf(fd, "update_progress: %s\n", ds_.options_->do_progress_updates ? "true" : "false"); @@ -154,8 +157,7 @@ status_t DumpstateService::dump(int fd, const Vector&) { dprintf(fd, "args: %s\n", ds_.options_->args.c_str()); dprintf(fd, "extra_options: %s\n", ds_.options_->extra_options.c_str()); dprintf(fd, "version: %s\n", ds_.version_.c_str()); - dprintf(fd, "bugreport_dir: %s\n", ds_.bugreport_dir_.c_str()); - dprintf(fd, "bugreport_internal_dir_: %s\n", ds_.bugreport_internal_dir_.c_str()); + dprintf(fd, "bugreport_dir: %s\n", destination.c_str()); dprintf(fd, "screenshot_path: %s\n", ds_.screenshot_path_.c_str()); dprintf(fd, "log_path: %s\n", ds_.log_path_.c_str()); dprintf(fd, "tmp_path: %s\n", ds_.tmp_path_.c_str()); diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 2824c82db1..b226a7d89e 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -135,10 +135,6 @@ static int Open(std::string path, int flags, mode_t mode = 0) { return fd; } -static int OpenForWrite(std::string path) { - return Open(path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, - S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); -} static int OpenForRead(std::string path) { return Open(path, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); @@ -169,17 +165,6 @@ static bool CopyFileToFd(const std::string& input_file, int out_fd) { return false; } -static bool CopyFileToFile(const std::string& input_file, const std::string& output_file) { - if (input_file == output_file) { - MYLOGD("Skipping copying bugreport file since the destination is the same (%s)\n", - output_file.c_str()); - return false; - } - - MYLOGD("Going to copy bugreport file (%s) to %s\n", input_file.c_str(), output_file.c_str()); - android::base::unique_fd out_fd(OpenForWrite(output_file)); - return CopyFileToFd(input_file, out_fd.get()); -} } // namespace } // namespace os @@ -1800,16 +1785,9 @@ static void MaybeResolveSymlink(std::string* path) { static void PrepareToWriteToFile() { MaybeResolveSymlink(&ds.bugreport_internal_dir_); - std::string base_name_part1 = "bugreport"; - if (!ds.options_->use_outfile.empty()) { - ds.bugreport_dir_ = dirname(ds.options_->use_outfile.c_str()); - base_name_part1 = basename(ds.options_->use_outfile.c_str()); - } - std::string build_id = android::base::GetProperty("ro.build.id", "UNKNOWN_BUILD"); std::string device_name = android::base::GetProperty("ro.product.name", "UNKNOWN_DEVICE"); - ds.base_name_ = - StringPrintf("%s-%s-%s", base_name_part1.c_str(), device_name.c_str(), build_id.c_str()); + ds.base_name_ = StringPrintf("bugreport-%s-%s", device_name.c_str(), build_id.c_str()); if (ds.options_->do_add_date) { char date[80]; strftime(date, sizeof(date), "%Y-%m-%d-%H-%M-%S", localtime(&ds.now_)); @@ -1832,17 +1810,16 @@ static void PrepareToWriteToFile() { std::string destination = ds.options_->bugreport_fd.get() != -1 ? StringPrintf("[fd:%d]", ds.options_->bugreport_fd.get()) - : ds.bugreport_dir_.c_str(); + : ds.bugreport_internal_dir_.c_str(); MYLOGD( "Bugreport dir: %s\n" - "Internal Bugreport dir: %s\n" "Base name: %s\n" "Suffix: %s\n" "Log path: %s\n" "Temporary path: %s\n" "Screenshot path: %s\n", - destination.c_str(), ds.bugreport_internal_dir_.c_str(), ds.base_name_.c_str(), - ds.name_.c_str(), ds.log_path_.c_str(), ds.tmp_path_.c_str(), ds.screenshot_path_.c_str()); + destination.c_str(), ds.base_name_.c_str(), ds.name_.c_str(), ds.log_path_.c_str(), + ds.tmp_path_.c_str(), ds.screenshot_path_.c_str()); if (ds.options_->do_zip_file) { ds.path_ = ds.GetPath(".zip"); @@ -1909,18 +1886,13 @@ static void FinalizeFile() { } } // The zip file lives in an internal directory. Copy it over to output. - bool copy_succeeded = false; if (ds.options_->bugreport_fd.get() != -1) { - copy_succeeded = android::os::CopyFileToFd(ds.path_, ds.options_->bugreport_fd.get()); - } else { - ds.final_path_ = ds.GetPath(ds.bugreport_dir_, ".zip"); - copy_succeeded = android::os::CopyFileToFile(ds.path_, ds.final_path_); - } - if (copy_succeeded) { - if (remove(ds.path_.c_str())) { + bool copy_succeeded = + android::os::CopyFileToFd(ds.path_, ds.options_->bugreport_fd.get()); + if (!copy_succeeded && remove(ds.path_.c_str())) { MYLOGE("remove(%s): %s", ds.path_.c_str(), strerror(errno)); } - } + } // else - the file just remains in the internal directory. } } if (do_text_file) { @@ -1946,8 +1918,8 @@ static void FinalizeFile() { /* Broadcasts that we are done with the bugreport */ static void SendBugreportFinishedBroadcast() { // TODO(b/111441001): use callback instead of broadcast. - if (!ds.final_path_.empty()) { - MYLOGI("Final bugreport path: %s\n", ds.final_path_.c_str()); + if (!ds.path_.empty()) { + MYLOGI("Final bugreport path: %s\n", ds.path_.c_str()); // clang-format off std::vector am_args = { @@ -1955,7 +1927,7 @@ static void SendBugreportFinishedBroadcast() { "--ei", "android.intent.extra.ID", std::to_string(ds.id_), "--ei", "android.intent.extra.PID", std::to_string(ds.pid_), "--ei", "android.intent.extra.MAX", std::to_string(ds.progress_->GetMax()), - "--es", "android.intent.extra.BUGREPORT", ds.final_path_, + "--es", "android.intent.extra.BUGREPORT", ds.path_, "--es", "android.intent.extra.DUMPSTATE_LOG", ds.log_path_ }; // clang-format on @@ -1977,7 +1949,7 @@ static void SendBugreportFinishedBroadcast() { if (ds.options_->is_remote_mode) { am_args.push_back("--es"); am_args.push_back("android.intent.extra.REMOTE_BUGREPORT_HASH"); - am_args.push_back(SHA256_file_hash(ds.final_path_)); + am_args.push_back(SHA256_file_hash(ds.path_)); SendBroadcast("com.android.internal.intent.action.REMOTE_BUGREPORT_FINISHED", am_args); } else { SendBroadcast("com.android.internal.intent.action.BUGREPORT_FINISHED", am_args); @@ -2115,7 +2087,6 @@ static void LogDumpOptions(const Dumpstate::DumpOptions& options) { MYLOGI("wifi_only: %d\n", options.wifi_only); MYLOGI("do_progress_updates: %d\n", options.do_progress_updates); MYLOGI("fd: %d\n", options.bugreport_fd.get()); - MYLOGI("use_outfile: %s\n", options.use_outfile.c_str()); MYLOGI("extra_options: %s\n", options.extra_options.c_str()); MYLOGI("args: %s\n", options.args.c_str()); MYLOGI("notification_title: %s\n", options.notification_title.c_str()); @@ -2146,7 +2117,9 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) // clang-format off case 'd': do_add_date = true; break; case 'z': do_zip_file = true; break; - case 'o': use_outfile = optarg; break; + // o=use_outfile not supported anymore. + // TODO(b/111441001): Remove when all callers have migrated. + case 'o': break; case 's': use_socket = true; break; case 'S': use_control_socket = true; break; case 'v': show_header_only = true; break; @@ -2187,9 +2160,7 @@ bool Dumpstate::DumpOptions::ValidateOptions() const { return false; } - bool has_out_file_options = !use_outfile.empty() || bugreport_fd.get() != -1; - if ((do_zip_file || do_add_date || do_progress_updates || do_broadcast) && - !has_out_file_options) { + if ((do_zip_file || do_add_date || do_progress_updates || do_broadcast) && !OutputToFile()) { return false; } @@ -2251,8 +2222,8 @@ Dumpstate::RunStatus Dumpstate::Run() { * If zipping, a bunch of other files and dumps also get added to the zip archive. The log file also * gets added to the archive. * - * Bugreports are first generated in a local directory and later copied to the caller's fd or - * directory. + * Bugreports are first generated in a local directory and later copied to the caller's fd if + * supplied. */ Dumpstate::RunStatus Dumpstate::RunInternal() { LogDumpOptions(*options_); @@ -2293,7 +2264,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal() { } // Redirect output if needed - bool is_redirecting = !options_->use_socket && !options_->use_outfile.empty(); + bool is_redirecting = options_->OutputToFile(); // TODO: temporarily set progress until it's part of the Dumpstate constructor std::string stats_path = @@ -2444,7 +2415,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal() { } /* rename or zip the (now complete) .tmp file to its final location */ - if (!options_->use_outfile.empty()) { + if (options_->OutputToFile()) { FinalizeFile(); } diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index c620c079a7..3dfe4e955f 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -331,6 +331,7 @@ class Dumpstate { bool do_add_date = false; bool do_zip_file = false; bool do_vibrate = true; + // Writes bugreport content to a socket; only flatfile format is supported. bool use_socket = false; bool use_control_socket = false; bool do_fb = false; @@ -342,13 +343,11 @@ class Dumpstate { bool wifi_only = false; // Whether progress updates should be published. bool do_progress_updates = false; - // File descriptor to output zip file. Takes precedence over use_outfile. + // File descriptor to output zip file. android::base::unique_fd bugreport_fd; // File descriptor to screenshot file. // TODO(b/111441001): Use this fd. android::base::unique_fd screenshot_fd; - // Partial path to output file. - std::string use_outfile; // TODO: rename to MODE. // Extra options passed as system property. std::string extra_options; @@ -367,6 +366,13 @@ class Dumpstate { /* Returns true if the options set so far are consistent. */ bool ValidateOptions() const; + + /* Returns if options specified require writing bugreport to a file */ + bool OutputToFile() const { + // If we are not writing to socket, we will write to a file. If bugreport_fd is + // specified, it is preferred. If not bugreport is written to /bugreports. + return !use_socket; + } }; // TODO: initialize fields on constructor @@ -424,13 +430,6 @@ class Dumpstate { // Full path of the temporary file containing the screenshot (when requested). std::string screenshot_path_; - // TODO(b/111441001): remove when obsolete. - // Full path of the final zip file inside the caller-specified directory, if available. - std::string final_path_; - - // The caller-specified directory, if available. - std::string bugreport_dir_; - // Pointer to the zipped file. std::unique_ptr zip_file{nullptr, fclose}; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index cd9b97ff9d..0302c46ae8 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -173,7 +173,6 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_add_date); EXPECT_FALSE(options_.do_zip_file); - EXPECT_EQ("", options_.use_outfile); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); EXPECT_FALSE(options_.show_header_only); @@ -191,7 +190,6 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { const_cast("-S"), const_cast("-d"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on @@ -201,7 +199,6 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.use_control_socket); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -228,7 +225,6 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { // Other options retain default values EXPECT_TRUE(options_.do_vibrate); - EXPECT_EQ("", options_.use_outfile); EXPECT_FALSE(options_.do_add_date); EXPECT_FALSE(options_.do_zip_file); EXPECT_FALSE(options_.use_control_socket); @@ -247,7 +243,6 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { const_cast("-p"), const_cast("-B"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on property_set("dumpstate.options", "bugreportfull"); @@ -259,7 +254,6 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { EXPECT_TRUE(options_.do_fb); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_broadcast); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -279,7 +273,6 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { const_cast("-p"), const_cast("-B"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on @@ -294,7 +287,6 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { EXPECT_TRUE(options_.do_progress_updates); EXPECT_TRUE(options_.do_start_service); EXPECT_FALSE(options_.do_fb); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -312,7 +304,6 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { const_cast("-p"), const_cast("-B"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on @@ -327,7 +318,6 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { EXPECT_TRUE(options_.is_remote_mode); EXPECT_FALSE(options_.do_vibrate); EXPECT_FALSE(options_.do_fb); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_FALSE(options_.use_control_socket); @@ -344,7 +334,6 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { const_cast("-p"), const_cast("-B"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on @@ -359,7 +348,6 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_progress_updates); EXPECT_TRUE(options_.do_start_service); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -377,7 +365,6 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { const_cast("-p"), const_cast("-B"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on @@ -391,7 +378,6 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.telephony_only); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -410,7 +396,6 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) { const_cast("-p"), const_cast("-B"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on @@ -424,7 +409,6 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) { EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.wifi_only); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -444,7 +428,6 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { const_cast("-p"), const_cast("-B"), const_cast("-z"), - const_cast("-o abc"), }; // clang-format on @@ -457,7 +440,6 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { EXPECT_TRUE(options_.do_fb); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_broadcast); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -475,7 +457,6 @@ TEST_F(DumpOptionsTest, InitializePartial1) { const_cast("dumpstate"), const_cast("-d"), const_cast("-z"), - const_cast("-o abc"), const_cast("-s"), const_cast("-S"), @@ -488,7 +469,6 @@ TEST_F(DumpOptionsTest, InitializePartial1) { EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_zip_file); // TODO: Maybe we should trim the filename - EXPECT_EQ(" abc", std::string(options_.use_outfile)); EXPECT_TRUE(options_.use_socket); EXPECT_TRUE(options_.use_control_socket); @@ -527,7 +507,6 @@ TEST_F(DumpOptionsTest, InitializePartial2) { // Other options retain default values EXPECT_FALSE(options_.do_add_date); EXPECT_FALSE(options_.do_zip_file); - EXPECT_EQ("", options_.use_outfile); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); } @@ -562,15 +541,21 @@ TEST_F(DumpOptionsTest, InitializeUnknown) { TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile1) { options_.do_zip_file = true; + // Writing to socket = !writing to file. + options_.use_socket = true; EXPECT_FALSE(options_.ValidateOptions()); - options_.use_outfile = "a/b/c"; + + options_.use_socket = false; EXPECT_TRUE(options_.ValidateOptions()); } TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile2) { options_.do_broadcast = true; + // Writing to socket = !writing to file. + options_.use_socket = true; EXPECT_FALSE(options_.ValidateOptions()); - options_.use_outfile = "a/b/c"; + + options_.use_socket = false; EXPECT_TRUE(options_.ValidateOptions()); } @@ -579,13 +564,11 @@ TEST_F(DumpOptionsTest, ValidateOptionsNeedZipfile) { EXPECT_FALSE(options_.ValidateOptions()); options_.do_zip_file = true; - options_.use_outfile = "a/b/c"; // do_zip_file needs outfile EXPECT_TRUE(options_.ValidateOptions()); } TEST_F(DumpOptionsTest, ValidateOptionsUpdateProgressNeedsBroadcast) { options_.do_progress_updates = true; - options_.use_outfile = "a/b/c"; // do_progress_updates needs outfile EXPECT_FALSE(options_.ValidateOptions()); options_.do_broadcast = true; @@ -599,7 +582,6 @@ TEST_F(DumpOptionsTest, ValidateOptionsRemoteMode) { options_.do_broadcast = true; options_.do_zip_file = true; options_.do_add_date = true; - options_.use_outfile = "a/b/c"; // do_broadcast needs outfile EXPECT_TRUE(options_.ValidateOptions()); } -- cgit v1.2.3-59-g8ed1b