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

Commit ea160d13 authored by Felipe Leme's avatar Felipe Leme
Browse files

Improved (or warned about lack of) error handling. 

It would be safer for dumpstate to exit when execvp on a child fails; a
common occurrence is when a list of command arguments is missing NULL.

dumpstate should be more robust to detect those missing NULL-terminated
args, but that will be addressed in a future change.

BUG: 27804637
BUG: 27832567
Change-Id: Ibcbe46041a86b16e365fbb40613b8c4bdf39744c
parent 950aa7c8

cmds/dumpstate/dumpstate.cpp

+3 −3
Original line number Diff line number Diff line
@@ -198,9 +198,9 @@ static void dump_systrace() { 
        // The drawback of calling execl directly is that we're not timing out if it hangs.

        MYLOGD("Running '/system/bin/atrace --async_dump', which can take several seconds");

        execl("/system/bin/atrace", "/system/bin/atrace", "--async_dump", nullptr);

        // execl should never return, but it doesn't hurt to handle that scenario

        MYLOGD("execl on '/system/bin/atrace --async_dump' returned control");

        return;

        // execl should never return, but if it did, we need to exit.

        MYLOGD("execl on '/system/bin/atrace --async_dump' failed: %s", strerror(errno));

        exit(EXIT_FAILURE);

    } else {

        close(pipefd[1]);  // close the write end of the pipe in the parent

        add_zip_entry_from_fd("systrace.txt", pipefd[0]); // write output to zip file

cmds/dumpstate/utils.cpp

+11 −3
Original line number Diff line number Diff line
@@ -648,6 +648,8 @@ int run_command(const char *title, int timeout_seconds, const char *command, ... 
            null_terminated = true;

            break;

        }

        // TODO: null_terminated check is not really working; line below would crash dumpstate if

        // nullptr is missing

        if (title) printf(" %s", args[arg]);

    }

    if (title) printf(") ------\n");
@@ -683,6 +685,8 @@ int run_command_as_shell(const char *title, int timeout_seconds, const char *com 
            null_terminated = true;

            break;

        }

        // TODO: null_terminated check is not really working; line below would crash dumpstate if

        // nullptr is missing

        if (title) printf(" %s", args[arg]);

    }

    if (title) printf(") ------\n");
@@ -704,6 +708,8 @@ int run_command_as_shell(const char *title, int timeout_seconds, const char *com 


/* forks a command and waits for it to finish */

int run_command_always(const char *title, bool drop_root, int timeout_seconds, const char *args[]) {

    // TODO: need to check if args is null-terminated, otherwise execvp will crash dumpstate



    /* TODO: for now we're simplifying the progress calculation by using the timeout as the weight.

     * It's a good approximation for most cases, except when calling dumpsys, where its weight

     * should be much higher proportionally to its timeout. */
@@ -736,10 +742,11 @@ int run_command_always(const char *title, bool drop_root, int timeout_seconds, c 
        sigaction(SIGPIPE, &sigact, NULL);



        execvp(command, (char**) args);

        // execvp's result will be handled after waitpid_with_timeout() below...

        MYLOGD("execvp on command %s returned control (error: %s)", command, strerror(errno));

        // execvp's result will be handled after waitpid_with_timeout() below, but if it failed,

        // it's safer to exit dumpstate.

        MYLOGD("execvp on command '%s' failed (error: %s)", command, strerror(errno));

        fflush(stdout);

        return -1; // ...but it doesn't hurt to force exit, just in case

        exit(EXIT_FAILURE);

    }



    /* handle parent case */
@@ -1354,5 +1361,6 @@ void format_args(const char* command, const char *args[], std::string *string) { 
            string->append(" ");

        }

    }

    // TODO: not really working: if NULL is missing, it will crash dumpstate.

    MYLOGE("internal error: missing NULL entry on %s", string->c_str());

}