From 1700cc46b5531499b7b532cda05d442f5f60acd4 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 16 Jul 2018 22:09:59 -0700 Subject: [PATCH] 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 --- otautil/include/otautil/sysutil.h | 5 +++++ otautil/sysutil.cpp | 9 +++++++++ recovery.cpp | 11 ++++++----- recovery_main.cpp | 7 ++----- tests/unit/sysutil_test.cpp | 10 ++++++++++ 5 files changed, 32 insertions(+), 10 deletions(-) diff --git a/otautil/include/otautil/sysutil.h b/otautil/include/otautil/sysutil.h index 649f8ffa..2eeb7c30 100644 --- a/otautil/include/otautil/sysutil.h +++ b/otautil/include/otautil/sysutil.h @@ -54,4 +54,9 @@ class MemMapping { // 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 StringVectorToNullTerminatedArray(const std::vector& args); + #endif // _OTAUTIL_SYSUTIL diff --git a/otautil/sysutil.cpp b/otautil/sysutil.cpp index ab151308..d8969a0b 100644 --- a/otautil/sysutil.cpp +++ b/otautil/sysutil.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -211,3 +212,11 @@ bool reboot(const std::string& command) { } return android::base::SetProperty(ANDROID_RB_PROPERTY, cmd); } + +std::vector StringVectorToNullTerminatedArray(const std::vector& args) { + std::vector result(args.size()); + std::transform(args.cbegin(), args.cend(), result.begin(), + [](const std::string& arg) { return const_cast(arg.c_str()); }); + result.push_back(nullptr); + return result; +} diff --git a/recovery.cpp b/recovery.cpp index fea65ae9..3284440d 100644 --- a/recovery.cpp +++ b/recovery.cpp @@ -963,10 +963,6 @@ static void log_failure_code(ErrorCode code, const std::string& update_package) } Device::BuiltinAction start_recovery(Device* device, const std::vector& args) { - std::vector args_to_parse(args.size()); - std::transform(args.cbegin(), args.cend(), args_to_parse.begin(), - [](const std::string& arg) { return const_cast(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 @@ Device::BuiltinAction start_recovery(Device* device, const std::vector #include -#include #include #include @@ -287,9 +286,7 @@ int main(int argc, char** argv) { has_cache = volume_for_mount_point(CACHE_ROOT) != nullptr; std::vector args = get_args(argc, argv); - std::vector args_to_parse(args.size()); - std::transform(args.cbegin(), args.cend(), args_to_parse.begin(), - [](const std::string& arg) { return const_cast(arg.c_str()); }); + auto args_to_parse = StringVectorToNullTerminatedArray(args); static constexpr struct option OPTIONS[] = { { "locale", required_argument, nullptr, 0 }, @@ -302,7 +299,7 @@ int main(int argc, char** argv) { 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 19fa4c59..de8ff706 100644 --- a/tests/unit/sysutil_test.cpp +++ b/tests/unit/sysutil_test.cpp @@ -127,3 +127,13 @@ TEST(SysUtilTest, MapFileBlockMapInvalidBlockMap) { 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 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]); +}