diff options
| -rw-r--r-- | cmds/dumpstate/DumpstateService.cpp | 62 | ||||
| -rw-r--r-- | cmds/dumpstate/binder/android/os/IDumpstate.aidl | 3 | ||||
| -rw-r--r-- | cmds/dumpstate/binder/android/os/IDumpstateListener.aidl | 17 | ||||
| -rw-r--r-- | cmds/dumpstate/dumpstate.cpp | 37 | ||||
| -rw-r--r-- | cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 216 |
5 files changed, 279 insertions, 56 deletions
diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index bb089e69b1..768cb4f2f6 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -44,17 +44,18 @@ static binder::Status exception(uint32_t code, const std::string& msg) { return binder::Status::fromExceptionCode(code, String8(msg.c_str())); } -static binder::Status error(uint32_t code, const std::string& msg) { - MYLOGE("%s (%d) ", msg.c_str(), code); - return binder::Status::fromServiceSpecificError(code, String8(msg.c_str())); -} - -// Takes ownership of data. -static void* callAndNotify(void* data) { +// Creates a bugreport and exits, thus preserving the oneshot nature of the service. +// Note: takes ownership of data. +[[noreturn]] static void* dumpstate_thread_main(void* data) { std::unique_ptr<DumpstateInfo> ds_info(static_cast<DumpstateInfo*>(data)); ds_info->ds->Run(ds_info->calling_uid, ds_info->calling_package); - MYLOGD("Finished Run()\n"); - return nullptr; + MYLOGD("Finished taking a bugreport. Exiting.\n"); + exit(0); +} + +[[noreturn]] static void signalErrorAndExit(sp<IDumpstateListener> listener, int error_code) { + listener->onError(error_code); + exit(0); } class DumpstateToken : public BnDumpstateToken {}; @@ -120,6 +121,25 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, const sp<IDumpstateListener>& listener) { MYLOGI("startBugreport() with mode: %d\n", bugreport_mode); + // This is the bugreporting API flow, so ensure there is only one bugreport in progress at a + // time. + std::lock_guard<std::mutex> lock(lock_); + if (ds_ != nullptr) { + MYLOGE("Error! There is already a bugreport in progress. Returning."); + if (listener != nullptr) { + listener->onError(IDumpstateListener::BUGREPORT_ERROR_CONCURRENT_BUGREPORTS_FORBIDDEN); + } + return exception(binder::Status::EX_SERVICE_SPECIFIC, + "There is already a bugreport in progress"); + } + + // From here on, all conditions that indicate we are done with this incoming request should + // result in exiting the service to free it up for next invocation. + if (listener == nullptr) { + MYLOGE("Invalid input: no listener"); + exit(0); + } + if (bugreport_mode != Dumpstate::BugreportMode::BUGREPORT_FULL && bugreport_mode != Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE && bugreport_mode != Dumpstate::BugreportMode::BUGREPORT_REMOTE && @@ -127,30 +147,23 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, bugreport_mode != Dumpstate::BugreportMode::BUGREPORT_TELEPHONY && bugreport_mode != Dumpstate::BugreportMode::BUGREPORT_WIFI && bugreport_mode != Dumpstate::BugreportMode::BUGREPORT_DEFAULT) { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, - StringPrintf("Invalid bugreport mode: %d", bugreport_mode)); + MYLOGE("Invalid input: bad bugreport mode: %d", bugreport_mode); + signalErrorAndExit(listener, IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT); } if (bugreport_fd.get() == -1 || screenshot_fd.get() == -1) { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Invalid file descriptor"); + // TODO(b/111441001): screenshot fd should be optional + MYLOGE("Invalid filedescriptor"); + signalErrorAndExit(listener, IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT); } std::unique_ptr<Dumpstate::DumpOptions> options = std::make_unique<Dumpstate::DumpOptions>(); options->Initialize(static_cast<Dumpstate::BugreportMode>(bugreport_mode), bugreport_fd, screenshot_fd); - // This is the bugreporting API flow, so ensure there is only one bugreport in progress at a - // time. - std::lock_guard<std::mutex> lock(lock_); - if (ds_ != nullptr) { - return exception(binder::Status::EX_SERVICE_SPECIFIC, - "There is already a bugreport in progress"); - } ds_ = &(Dumpstate::GetInstance()); ds_->SetOptions(std::move(options)); - if (listener != nullptr) { - ds_->listener_ = listener; - } + ds_->listener_ = listener; DumpstateInfo* ds_info = new DumpstateInfo(); ds_info->ds = ds_; @@ -158,11 +171,12 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, ds_info->calling_package = calling_package; pthread_t thread; - status_t err = pthread_create(&thread, nullptr, callAndNotify, ds_info); + status_t err = pthread_create(&thread, nullptr, dumpstate_thread_main, ds_info); if (err != 0) { delete ds_info; ds_info = nullptr; - return error(err, "Could not create a background thread."); + MYLOGE("Could not create a thread"); + signalErrorAndExit(listener, IDumpstateListener::BUGREPORT_ERROR_RUNTIME_ERROR); } return binder::Status::ok(); } diff --git a/cmds/dumpstate/binder/android/os/IDumpstate.aidl b/cmds/dumpstate/binder/android/os/IDumpstate.aidl index b1005d3885..37ff4428a1 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstate.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstate.aidl @@ -36,6 +36,9 @@ interface IDumpstate { IDumpstateToken setListener(@utf8InCpp String name, IDumpstateListener listener, boolean getSectionDetails); + // NOTE: If you add to or change these modes, please also change the corresponding enums + // in system server, in BugreportParams.java. + // These modes encapsulate a set of run time options for generating bugreports. // Takes a bugreport without user interference. const int BUGREPORT_MODE_FULL = 0; diff --git a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl index 907a67c907..42858e0827 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl @@ -19,6 +19,11 @@ package android.os; /** * Listener for dumpstate events. * + * <p>When bugreport creation is complete one of {@code onError} or {@code onFinished} is called. + * + * <p>These methods are synchronous by design in order to make dumpstate's lifecycle simpler + * to handle. + * * {@hide} */ interface IDumpstateListener { @@ -27,7 +32,10 @@ interface IDumpstateListener { * * @param progress the progress in [0, 100] */ - oneway void onProgress(int progress); + void onProgress(int progress); + + // NOTE: If you add to or change these error codes, please also change the corresponding enums + // in system server, in BugreportManager.java. /* Options specified are invalid or incompatible */ const int BUGREPORT_ERROR_INVALID_INPUT = 1; @@ -41,15 +49,18 @@ interface IDumpstateListener { /* The request to get user consent timed out */ const int BUGREPORT_ERROR_USER_CONSENT_TIMED_OUT = 4; + /* There is currently a bugreport running. The caller should try again later. */ + const int BUGREPORT_ERROR_CONCURRENT_BUGREPORTS_FORBIDDEN = 5; + /** * Called on an error condition with one of the error codes listed above. */ - oneway void onError(int errorCode); + void onError(int errorCode); /** * Called when taking bugreport finishes successfully. */ - oneway void onFinished(); + void onFinished(); // TODO(b/111441001): Remove old methods when not used anymore. void onProgressUpdated(int progress); diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index d3e0b935bb..90c16094d8 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -158,7 +158,7 @@ bool CopyFile(int in_fd, int out_fd) { } static bool CopyFileToFd(const std::string& input_file, int out_fd) { - MYLOGD("Going to copy bugreport file (%s) to %d\n", ds.path_.c_str(), out_fd); + MYLOGD("Going to copy file (%s) to %d\n", input_file.c_str(), out_fd); // Obtain a handle to the source file. android::base::unique_fd in_fd(OpenForRead(input_file)); @@ -166,14 +166,14 @@ static bool CopyFileToFd(const std::string& input_file, int out_fd) { if (CopyFile(in_fd.get(), out_fd)) { return true; } - MYLOGE("Failed to copy zip file: %s\n", strerror(errno)); + MYLOGE("Failed to copy file: %s\n", strerror(errno)); } return false; } static bool UnlinkAndLogOnError(const std::string& file) { - if (unlink(file.c_str()) != -1) { - MYLOGE("Failed to remove file (%s): %s\n", file.c_str(), strerror(errno)); + if (unlink(file.c_str())) { + MYLOGE("Failed to unlink file (%s): %s\n", file.c_str(), strerror(errno)); return false; } return true; @@ -461,9 +461,7 @@ static bool dump_anrd_trace() { if (!ds.AddZipEntry("anrd_trace.txt", path)) { MYLOGE("Unable to add anrd_trace file %s to zip file\n", path); } else { - if (remove(path)) { - MYLOGE("Error removing anrd_trace file %s: %s", path, strerror(errno)); - } + android::os::UnlinkAndLogOnError(path); return true; } } else { @@ -1587,13 +1585,8 @@ void Dumpstate::DumpstateBoard() { for (int i = 0; i < NUM_OF_DUMPS; i++) { paths.emplace_back(StringPrintf("%s/%s", ds.bugreport_internal_dir_.c_str(), kDumpstateBoardFiles[i].c_str())); - remover.emplace_back(android::base::make_scope_guard(std::bind( - [](std::string path) { - if (remove(path.c_str()) != 0 && errno != ENOENT) { - MYLOGE("Could not remove(%s): %s\n", path.c_str(), strerror(errno)); - } - }, - paths[i]))); + remover.emplace_back(android::base::make_scope_guard( + std::bind([](std::string path) { android::os::UnlinkAndLogOnError(path); }, paths[i]))); } sp<IDumpstateDevice> dumpstate_device(IDumpstateDevice::getService()); @@ -1754,9 +1747,7 @@ bool Dumpstate::FinishZipFile() { ds.zip_file.reset(nullptr); MYLOGD("Removing temporary file %s\n", tmp_path_.c_str()) - if (remove(tmp_path_.c_str()) != 0) { - MYLOGE("Failed to remove temporary file (%s): %s\n", tmp_path_.c_str(), strerror(errno)); - } + android::os::UnlinkAndLogOnError(tmp_path_); return true; } @@ -2340,6 +2331,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, register_sig_handler(); + // TODO(b/111441001): maybe skip if already started? if (options_->do_start_service) { MYLOGI("Starting 'dumpstate' service\n"); android::status_t ret; @@ -2486,15 +2478,24 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // Do an early return if there were errors. We make an exception for consent // timing out because it's possible the user got distracted. In this case the // bugreport is not shared but made available for manual retrieval. + MYLOGI("User denied consent. Returning\n"); return status; } - if (options_->screenshot_fd.get() != -1) { + if (options_->do_fb && options_->screenshot_fd.get() != -1) { bool copy_succeeded = android::os::CopyFileToFd(screenshot_path_, options_->screenshot_fd.get()); if (copy_succeeded) { android::os::UnlinkAndLogOnError(screenshot_path_); } } + if (status == Dumpstate::RunStatus::USER_CONSENT_TIMED_OUT) { + MYLOGI( + "Did not receive user consent yet." + " Will not copy the bugreport artifacts to caller.\n"); + // TODO(b/111441001): + // 1. cancel outstanding requests + // 2. check for result more frequently + } } /* vibrate a few but shortly times to let user know it's finished */ diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp index 9f99114271..555badd61b 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -21,6 +21,10 @@ #include <libgen.h> #include <android-base/file.h> +#include <android/os/BnDumpstate.h> +#include <android/os/BnDumpstateListener.h> +#include <binder/IServiceManager.h> +#include <binder/ProcessState.h> #include <cutils/properties.h> #include <ziparchive/zip_archive.h> @@ -34,6 +38,24 @@ namespace dumpstate { using ::testing::Test; using ::std::literals::chrono_literals::operator""s; +using android::base::unique_fd; + +class DumpstateListener; + +namespace { + +sp<IDumpstate> GetDumpstateService() { + return android::interface_cast<IDumpstate>( + android::defaultServiceManager()->getService(String16("dumpstate"))); +} + +int OpenForWrite(const std::string& filename) { + return TEMP_FAILURE_RETRY(open(filename.c_str(), + O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, + S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)); +} + +} // namespace struct SectionInfo { std::string name; @@ -46,41 +68,71 @@ struct SectionInfo { * Listens to bugreport progress and updates the user by writing the progress to STDOUT. All the * section details generated by dumpstate are added to a vector to be used by Tests later. */ -class DumpstateListener : public IDumpstateListener { +class DumpstateListener : public BnDumpstateListener { public: - int outFd_, max_progress_; - std::shared_ptr<std::vector<SectionInfo>> sections_; DumpstateListener(int fd, std::shared_ptr<std::vector<SectionInfo>> sections) - : outFd_(fd), max_progress_(5000), sections_(sections) { + : out_fd_(fd), sections_(sections) { + } + + DumpstateListener(int fd) : out_fd_(fd) { } + binder::Status onProgress(int32_t progress) override { - dprintf(outFd_, "\rIn progress %d", progress); + dprintf(out_fd_, "\rIn progress %d", progress); return binder::Status::ok(); } + binder::Status onError(int32_t error_code) override { - dprintf(outFd_, "\rError %d", error_code); + std::lock_guard<std::mutex> lock(lock_); + error_code_ = error_code; + dprintf(out_fd_, "\rError code %d", error_code); return binder::Status::ok(); } + binder::Status onFinished() override { - dprintf(outFd_, "\rFinished"); + std::lock_guard<std::mutex> lock(lock_); + is_finished_ = true; + dprintf(out_fd_, "\rFinished"); return binder::Status::ok(); } + binder::Status onProgressUpdated(int32_t progress) override { - dprintf(outFd_, "\rIn progress %d/%d", progress, max_progress_); + dprintf(out_fd_, "\rIn progress %d/%d", progress, max_progress_); return binder::Status::ok(); } + binder::Status onMaxProgressUpdated(int32_t max_progress) override { + std::lock_guard<std::mutex> lock(lock_); max_progress_ = max_progress; return binder::Status::ok(); } + binder::Status onSectionComplete(const ::std::string& name, int32_t status, int32_t size_bytes, int32_t duration_ms) override { - sections_->push_back({name, status, size_bytes, duration_ms}); + std::lock_guard<std::mutex> lock(lock_); + if (sections_.get() != nullptr) { + sections_->push_back({name, status, size_bytes, duration_ms}); + } return binder::Status::ok(); } - IBinder* onAsBinder() override { - return nullptr; + + bool getIsFinished() { + std::lock_guard<std::mutex> lock(lock_); + return is_finished_; } + + int getErrorCode() { + std::lock_guard<std::mutex> lock(lock_); + return error_code_; + } + + private: + int out_fd_; + int max_progress_ = 5000; + int error_code_ = -1; + bool is_finished_ = false; + std::shared_ptr<std::vector<SectionInfo>> sections_; + std::mutex lock_; }; /** @@ -293,6 +345,148 @@ TEST_F(BugreportSectionTest, WifiSectionGenerated) { SectionExists("DUMPSYS - wifi", /* bytes= */ 100000); } +class DumpstateBinderTest : public Test { + protected: + void SetUp() override { + // In case there is a stray service, stop it first. + property_set("ctl.stop", "bugreportd"); + // dry_run results in a faster bugreport. + property_set("dumpstate.dry_run", "true"); + // We need to receive some async calls later. Ensure we have binder threads. + ProcessState::self()->startThreadPool(); + } + + void TearDown() override { + property_set("ctl.stop", "bugreportd"); + property_set("dumpstate.dry_run", ""); + + unlink("/data/local/tmp/tmp.zip"); + unlink("/data/local/tmp/tmp.png"); + } + + // Waits until listener gets the callbacks. + void WaitTillExecutionComplete(DumpstateListener* listener) { + // Wait till one of finished, error or timeout. + static const int kBugreportTimeoutSeconds = 120; + int i = 0; + while (!listener->getIsFinished() && listener->getErrorCode() == -1 && + i < kBugreportTimeoutSeconds) { + sleep(1); + i++; + } + } +}; + +TEST_F(DumpstateBinderTest, Baseline) { + // In the beginning dumpstate binder service is not running. + sp<android::os::IDumpstate> ds_binder(GetDumpstateService()); + EXPECT_EQ(ds_binder, nullptr); + + // Start bugreportd, which runs dumpstate binary with -w; which starts dumpstate service + // and makes it wait. + property_set("dumpstate.dry_run", "true"); + property_set("ctl.start", "bugreportd"); + + // Now we are able to retrieve dumpstate binder service. + ds_binder = GetDumpstateService(); + EXPECT_NE(ds_binder, nullptr); + + // Prepare arguments + unique_fd bugreport_fd(OpenForWrite("/bugreports/tmp.zip")); + unique_fd screenshot_fd(OpenForWrite("/bugreports/tmp.png")); + + EXPECT_NE(bugreport_fd.get(), -1); + EXPECT_NE(screenshot_fd.get(), -1); + + sp<DumpstateListener> listener(new DumpstateListener(dup(fileno(stdout)))); + android::binder::Status status = + ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd, + Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener); + // startBugreport is an async call. Verify binder call succeeded first, then wait till listener + // gets expected callbacks. + EXPECT_TRUE(status.isOk()); + WaitTillExecutionComplete(listener.get()); + + // Bugreport generation requires user consent, which we cannot get in a test set up, + // so instead of getting is_finished_, we are more likely to get a consent error. + EXPECT_TRUE( + listener->getErrorCode() == IDumpstateListener::BUGREPORT_ERROR_USER_DENIED_CONSENT || + listener->getErrorCode() == IDumpstateListener::BUGREPORT_ERROR_USER_CONSENT_TIMED_OUT); + + // The service should have died on its own, freeing itself up for a new invocation. + sleep(2); + ds_binder = GetDumpstateService(); + EXPECT_EQ(ds_binder, nullptr); +} + +TEST_F(DumpstateBinderTest, ServiceDies_OnInvalidInput) { + // Start bugreportd, which runs dumpstate binary with -w; which starts dumpstate service + // and makes it wait. + property_set("ctl.start", "bugreportd"); + sp<android::os::IDumpstate> ds_binder(GetDumpstateService()); + EXPECT_NE(ds_binder, nullptr); + + // Prepare arguments + unique_fd bugreport_fd(OpenForWrite("/data/local/tmp/tmp.zip")); + unique_fd screenshot_fd(OpenForWrite("/data/local/tmp/tmp.png")); + + EXPECT_NE(bugreport_fd.get(), -1); + EXPECT_NE(screenshot_fd.get(), -1); + + // Call startBugreport with bad arguments. + sp<DumpstateListener> listener(new DumpstateListener(dup(fileno(stdout)))); + android::binder::Status status = + ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd, + 2000, // invalid bugreport mode + listener); + EXPECT_EQ(listener->getErrorCode(), IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT); + + // The service should have died, freeing itself up for a new invocation. + sleep(2); + ds_binder = GetDumpstateService(); + EXPECT_EQ(ds_binder, nullptr); +} + +TEST_F(DumpstateBinderTest, SimultaneousBugreportsNotAllowed) { + // Start bugreportd, which runs dumpstate binary with -w; which starts dumpstate service + // and makes it wait. + property_set("dumpstate.dry_run", "true"); + property_set("ctl.start", "bugreportd"); + sp<android::os::IDumpstate> ds_binder(GetDumpstateService()); + EXPECT_NE(ds_binder, nullptr); + + // Prepare arguments + unique_fd bugreport_fd(OpenForWrite("/data/local/tmp/tmp.zip")); + unique_fd screenshot_fd(OpenForWrite("/data/local/tmp/tmp.png")); + + EXPECT_NE(bugreport_fd.get(), -1); + EXPECT_NE(screenshot_fd.get(), -1); + + sp<DumpstateListener> listener1(new DumpstateListener(dup(fileno(stdout)))); + android::binder::Status status = + ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd, + Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener1); + EXPECT_TRUE(status.isOk()); + + // try to make another call to startBugreport. This should fail. + sp<DumpstateListener> listener2(new DumpstateListener(dup(fileno(stdout)))); + status = ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd, + Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener2); + EXPECT_FALSE(status.isOk()); + WaitTillExecutionComplete(listener2.get()); + EXPECT_EQ(listener2->getErrorCode(), + IDumpstateListener::BUGREPORT_ERROR_CONCURRENT_BUGREPORTS_FORBIDDEN); + + // Meanwhile the first call works as expected. Service should not die in this case. + WaitTillExecutionComplete(listener1.get()); + + // Bugreport generation requires user consent, which we cannot get in a test set up, + // so instead of getting is_finished_, we are more likely to get a consent error. + EXPECT_TRUE( + listener1->getErrorCode() == IDumpstateListener::BUGREPORT_ERROR_USER_DENIED_CONSENT || + listener1->getErrorCode() == IDumpstateListener::BUGREPORT_ERROR_USER_CONSENT_TIMED_OUT); +} + } // namespace dumpstate } // namespace os } // namespace android |