Merge "Fix the arguments passed to getopt_long(3)."

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<char*> StringVectorToNullTerminatedArray(const std::vector<std::string>& args);
+
 #endif  // _OTAUTIL_SYSUTIL

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 @@ bool reboot(const std::string& command) {
   }
   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;
+}

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<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 @@ Device::BuiltinAction start_recovery(Device* device, const std::vector<std::stri
   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':

recovery_main.cpp

@@ -29,7 +29,6 @@
 #include <time.h>
 #include <unistd.h>
 
-#include <algorithm>
 #include <string>
 #include <vector>
 
@@ -287,9 +286,7 @@ int main(int argc, char** argv) {
   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 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':

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<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]);
+}