Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a407fb81 authored by Abhijeet Kaur's avatar Abhijeet Kaur
Browse files

Update Last ID early in the bugreport generation 

The lifecycle of a bugreport triggered by Shell:

1. Triggers a bugreport using bugreport API. At this point shell does not
have any state of the bugreport. mBugreportInfos is the sparse array in
Shell that tracks bugreportInfos keyed with the ID. The ID of the
bugreport is currently incremented in RunInternal() which is called in a
different thread triggered by the DumpstateService (for an API call)
2. dumpstate calls one of the callbacks #onProgress, #onError and
 #onFinished(), it is then that the corresponding bugreportinfo is
tracked in mBugreportInfos keyed with the ID.

Problem: Consider a case when 2 bugreports are triggered such that Shell
has not received #onProgress for the first one, and received #onError
for the second one. This results in a race condition as Shell assumes
that the first bugreport has called onError. This race condition occurs
due to the fact that Shell depends on the callback to track the
bugreports.

Solution: Update Id at a definite time (not in some other thread) in
Initialize so that the service calling the API can track it right away.

Bug: 152292912
Test: Tigger consecutive calls to bugreport from ActivityManager is WAI
Test: adb bugreport (open the finished bugreport and check that the
correct ID is shown)
Test: atest dumpstate_test

Change-Id: I63966800725d2aaa1d1d9d6eb997b86d564acf44
parent a02bd194

cmds/dumpstate/DumpstateService.cpp

+2 −0
Original line number Diff line number Diff line
@@ -137,6 +137,8 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, 
    ds_info->calling_package = calling_package;



    pthread_t thread;

    // Initialize dumpstate

    ds_->Initialize();

    status_t err = pthread_create(&thread, nullptr, dumpstate_thread_main, ds_info);

    if (err != 0) {

        delete ds_info;

cmds/dumpstate/dumpstate.cpp

+10 −7
Original line number Diff line number Diff line
@@ -2395,6 +2395,13 @@ void Dumpstate::SetOptions(std::unique_ptr<DumpOptions> options) { 
    options_ = std::move(options);

}



void Dumpstate::Initialize() {

    /* gets the sequential id */

    uint32_t last_id = android::base::GetIntProperty(PROPERTY_LAST_ID, 0);

    id_ = ++last_id;

    android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id));

}



Dumpstate::RunStatus Dumpstate::Run(int32_t calling_uid, const std::string& calling_package) {

    Dumpstate::RunStatus status = RunInternal(calling_uid, calling_package);

    if (listener_ != nullptr) {
@@ -2505,11 +2512,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, 
            : "";

    progress_.reset(new Progress(stats_path));



    /* gets the sequential id */

    uint32_t last_id = android::base::GetIntProperty(PROPERTY_LAST_ID, 0);

    id_ = ++last_id;

    android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id));



    if (acquire_wake_lock(PARTIAL_WAKE_LOCK, WAKE_LOCK_NAME) < 0) {

        MYLOGE("Failed to acquire wake lock: %s\n", strerror(errno));

    } else {
@@ -2837,8 +2839,9 @@ Dumpstate::RunStatus Dumpstate::ParseCommandlineAndRun(int argc, char* argv[]) { 
        assert(options_->bugreport_fd.get() == -1);



        // calling_uid and calling_package are for user consent to share the bugreport with

        // an app; they are irrelvant here because bugreport is only written to a local

        // directory, and not shared.

        // an app; they are irrelevant here because bugreport is triggered via command line.

        // Update Last ID before calling Run().

        Initialize();

        status = Run(-1 /* calling_uid */, "" /* calling_package */);

    }

    return status;

cmds/dumpstate/dumpstate.h

+9 −1
Original line number Diff line number Diff line
@@ -216,6 +216,9 @@ class Dumpstate { 
    /* Checkes whether dumpstate is generating a zipped bugreport. */

    bool IsZipping() const;



    /* Initialize dumpstate fields before starting bugreport generation */

    void Initialize();



    /*

     * Forks a command, waits for it to finish, and returns its status.

     *
@@ -329,7 +332,12 @@ class Dumpstate { 


    struct DumpOptions;



    /* Main entry point for running a complete bugreport. */

    /*

     * Main entry point for running a complete bugreport.

     *

     * Initialize() dumpstate before calling this method.

     *

     */

    RunStatus Run(int32_t calling_uid, const std::string& calling_package);



    RunStatus ParseCommandlineAndRun(int argc, char* argv[]);