Fix the arguments passed to getopt_long(3).
The getopt_long(3) implementation in Android (upstream freebsd) expects
a null-terminated array while parsing long options with required args.
if (long_options[match].has_arg == required_argument) {
optarg = nargv[optind++];
}
...
if (long_options[match].has_arg == required_argument && optarg == NULL) {
return (BADARG);
}
This seems to make sense in practice, as getopt(3) takes the first two
arguments of argc and argv that are "as passed to the main() function on
program invocation", and both of C and C++ spec say "the value of
argv[argc] shall be 0".
Prior to the CL, we may run into undefined behavior on malformed input
command line (e.g. missing arg for an option that requires one). This CL
fixes the issue by always appending a nullptr to the argument list (but
without counting that into argc).
Test: Build and boot into recovery with commands.
Change-Id: Ic6c37548f4db2f30aeabd40f387ca916eeca5392
diff --git a/otautil/include/otautil/sysutil.h b/otautil/include/otautil/sysutil.h
index 649f8ff..2eeb7c3 100644
--- a/otautil/include/otautil/sysutil.h
+++ b/otautil/include/otautil/sysutil.h
@@ -54,4 +54,9 @@
// command should start with "reboot," (e.g. "reboot,bootloader" or "reboot,").
bool reboot(const std::string& command);
+// Returns a null-terminated char* array, where the elements point to the C-strings in the given
+// vector, plus an additional nullptr at the end. This is a helper function that facilitates
+// calling C functions (such as getopt(3)) that expect an array of C-strings.
+std::vector<char*> StringVectorToNullTerminatedArray(const std::vector<std::string>& args);
+
#endif // _OTAUTIL_SYSUTIL
diff --git a/otautil/sysutil.cpp b/otautil/sysutil.cpp
index ab15130..d8969a0 100644
--- a/otautil/sysutil.cpp
+++ b/otautil/sysutil.cpp
@@ -23,6 +23,7 @@
#include <sys/stat.h>
#include <sys/types.h>
+#include <algorithm>
#include <string>
#include <vector>
@@ -211,3 +212,11 @@
}
return android::base::SetProperty(ANDROID_RB_PROPERTY, cmd);
}
+
+std::vector<char*> StringVectorToNullTerminatedArray(const std::vector<std::string>& args) {
+ std::vector<char*> result(args.size());
+ std::transform(args.cbegin(), args.cend(), result.begin(),
+ [](const std::string& arg) { return const_cast<char*>(arg.c_str()); });
+ result.push_back(nullptr);
+ return result;
+}
diff --git a/recovery.cpp b/recovery.cpp
index fea65ae..3284440 100644
--- a/recovery.cpp
+++ b/recovery.cpp
@@ -963,10 +963,6 @@
}
Device::BuiltinAction start_recovery(Device* device, const std::vector<std::string>& args) {
- std::vector<char*> args_to_parse(args.size());
- std::transform(args.cbegin(), args.cend(), args_to_parse.begin(),
- [](const std::string& arg) { return const_cast<char*>(arg.c_str()); });
-
static constexpr struct option OPTIONS[] = {
{ "fsck_unshare_blocks", no_argument, nullptr, 0 },
{ "just_exit", no_argument, nullptr, 'x' },
@@ -1002,9 +998,14 @@
bool security_update = false;
std::string locale;
+ auto args_to_parse = StringVectorToNullTerminatedArray(args);
+
int arg;
int option_index;
- while ((arg = getopt_long(args_to_parse.size(), args_to_parse.data(), "", OPTIONS,
+ // Parse everything before the last element (which must be a nullptr). getopt_long(3) expects a
+ // null-terminated char* array, but without counting null as an arg (i.e. argv[argc] should be
+ // nullptr).
+ while ((arg = getopt_long(args_to_parse.size() - 1, args_to_parse.data(), "", OPTIONS,
&option_index)) != -1) {
switch (arg) {
case 't':
diff --git a/recovery_main.cpp b/recovery_main.cpp
index 5e82c6c..c79d7d8 100644
--- a/recovery_main.cpp
+++ b/recovery_main.cpp
@@ -29,7 +29,6 @@
#include <time.h>
#include <unistd.h>
-#include <algorithm>
#include <string>
#include <vector>
@@ -287,9 +286,7 @@
has_cache = volume_for_mount_point(CACHE_ROOT) != nullptr;
std::vector<std::string> args = get_args(argc, argv);
- std::vector<char*> args_to_parse(args.size());
- std::transform(args.cbegin(), args.cend(), args_to_parse.begin(),
- [](const std::string& arg) { return const_cast<char*>(arg.c_str()); });
+ auto args_to_parse = StringVectorToNullTerminatedArray(args);
static constexpr struct option OPTIONS[] = {
{ "locale", required_argument, nullptr, 0 },
@@ -302,7 +299,7 @@
int arg;
int option_index;
- while ((arg = getopt_long(args_to_parse.size(), args_to_parse.data(), "", OPTIONS,
+ while ((arg = getopt_long(args_to_parse.size() - 1, args_to_parse.data(), "", OPTIONS,
&option_index)) != -1) {
switch (arg) {
case 't':
diff --git a/tests/unit/sysutil_test.cpp b/tests/unit/sysutil_test.cpp
index 19fa4c5..de8ff70 100644
--- a/tests/unit/sysutil_test.cpp
+++ b/tests/unit/sysutil_test.cpp
@@ -127,3 +127,13 @@
ASSERT_TRUE(android::base::WriteStringToFile("/doesntexist\n4096 4096\n1\n0 1\n", temp_file.path));
ASSERT_FALSE(mapping.MapFile(filename));
}
+
+TEST(SysUtilTest, StringVectorToNullTerminatedArray) {
+ std::vector<std::string> args{ "foo", "bar", "baz" };
+ auto args_with_nullptr = StringVectorToNullTerminatedArray(args);
+ ASSERT_EQ(4, args_with_nullptr.size());
+ ASSERT_STREQ("foo", args_with_nullptr[0]);
+ ASSERT_STREQ("bar", args_with_nullptr[1]);
+ ASSERT_STREQ("baz", args_with_nullptr[2]);
+ ASSERT_EQ(nullptr, args_with_nullptr[3]);
+}