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
diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp
index 1d5b738..ba9a967 100644
--- a/cmds/dumpstate/DumpstateService.cpp
+++ b/cmds/dumpstate/DumpstateService.cpp
@@ -144,6 +144,9 @@
}
status_t DumpstateService::dump(int fd, const Vector<String16>&) {
+ 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 @@
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 2824c82..b226a7d 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -135,10 +135,6 @@
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 @@
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 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 @@
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 @@
}
}
// 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 @@
/* 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<std::string> am_args = {
@@ -1955,7 +1927,7 @@
"--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 @@
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 @@
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 @@
// 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 @@
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 @@
* 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 @@
}
// 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 @@
}
/* 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 c620c07..3dfe4e9 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -331,6 +331,7 @@
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 @@
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 @@
/* 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 @@
// 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<FILE, int (*)(FILE*)> zip_file{nullptr, fclose};
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index cd9b97f..0302c46 100644
--- a/cmds/dumpstate/tests/dumpstate_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_test.cpp
@@ -173,7 +173,6 @@
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 @@
const_cast<char*>("-S"),
const_cast<char*>("-d"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -201,7 +199,6 @@
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 @@
// 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 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
property_set("dumpstate.options", "bugreportfull");
@@ -259,7 +254,6 @@
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 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -294,7 +287,6 @@
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 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -327,7 +318,6 @@
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 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -359,7 +348,6 @@
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 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -391,7 +378,6 @@
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 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -424,7 +409,6 @@
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 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -457,7 +440,6 @@
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 @@
const_cast<char*>("dumpstate"),
const_cast<char*>("-d"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
const_cast<char*>("-s"),
const_cast<char*>("-S"),
@@ -488,7 +469,6 @@
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 @@
// 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, 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 @@
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 @@
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());
}