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
diff --git a/cmds/dumpstate/utils.cpp b/cmds/dumpstate/utils.cpp
index 975cd27..8129852 100644
--- a/cmds/dumpstate/utils.cpp
+++ b/cmds/dumpstate/utils.cpp
@@ -648,6 +648,8 @@
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 @@
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 @@
/* 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 @@
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 @@
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());
}