Merge the three RAII vectors managing args lifetime

Having three different vectors littered over the method to manage
various parts of the lifetime of the argument vector we are creating is
a bit dangerous as it means a simple code change could result in a
desync of these three, so by moving the functionality of them all into a
wrapper class should prevent us from making such mistakes.

1 files changed, 108 insertions, 90 deletions

diff --git a/apt-pkg/deb/dpkgpm.cc b/apt-pkg/deb/dpkgpm.cc
index bdaa353c5..af288b640 100644
--- a/apt-pkg/deb/dpkgpm.cc
+++ b/apt-pkg/deb/dpkgpm.cc
@@ -1455,6 +1455,61 @@ bool pkgDPkgPM::ExpandPendingCalls(std::vector<Item> &List, pkgDepCache &Cache)
    }
    return true;
 }
+class APT_HIDDEN BuildDpkgCall {
+   std::vector<char*> args;
+   std::vector<bool> to_free;
+   size_t baseArguments = 0;
+   size_t baseArgumentsLen = 0;
+   size_t len = 0;
+public:
+   void clearCallArguments() {
+      for (size_t i = baseArguments; i < args.size(); ++i)
+	 if (to_free[i])
+	    std::free(args[i]);
+      args.erase(args.begin() + baseArguments, args.end());
+      to_free.erase(to_free.begin() + baseArguments, to_free.end());
+      len = baseArgumentsLen;
+   }
+   void reserve(size_t const n) {
+      args.reserve(n);
+      to_free.reserve(n);
+   }
+   void push_back(char const * const str) {
+      args.push_back(const_cast<char*>(str));
+      to_free.push_back(false);
+      len += strlen(args.back());
+   }
+   void push_back(std::string &&str) {
+      args.push_back(strdup(str.c_str()));
+      to_free.push_back(true);
+      len += str.length();
+   }
+   auto bytes() const { return len; }
+   auto data() const { return args.data(); }
+   auto begin() const { return args.cbegin(); }
+   auto end() const { return args.cend(); }
+   auto& front() const { return args.front(); }
+   APT_NORETURN void execute(char const *const errmsg) {
+      args.push_back(nullptr);
+      execvp(args.front(), &args.front());
+      std::cerr << errmsg << std::endl;
+      _exit(100);
+   }
+   BuildDpkgCall() {
+      for (auto &&arg : debSystem::GetDpkgBaseCommand())
+	 push_back(std::move(arg));
+      baseArguments = args.size();
+      baseArgumentsLen = len;
+   }
+   BuildDpkgCall(BuildDpkgCall const &) = delete;
+   BuildDpkgCall(BuildDpkgCall &&) = delete;
+   BuildDpkgCall& operator=(BuildDpkgCall const &) = delete;
+   BuildDpkgCall& operator=(BuildDpkgCall &&) = delete;
+   ~BuildDpkgCall() {
+      baseArguments = 0;
+      clearCallArguments();
+   }
+};
 bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 {
    struct Inhibitor
@@ -1494,18 +1549,6 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
    pkgPackageManager::SigINTStop = false;
    d->progress = progress;
 
-   // Generate the base argument list for dpkg
-   std::vector<std::string> const sArgs = debSystem::GetDpkgBaseCommand();
-   std::vector<const char *> Args(sArgs.size(), NULL);
-   std::transform(sArgs.begin(), sArgs.end(), Args.begin(),
-	 [](std::string const &s) { return s.c_str(); });
-   unsigned long long const StartSize = std::accumulate(sArgs.begin(), sArgs.end(), 0llu,
-	 [](unsigned long long const i, std::string const &s) { return i + s.length(); });
-   size_t const BaseArgs = Args.size();
-
-   fd_set rfds;
-   struct timespec tv;
-
    // try to figure out the max environment size
    int OSArgMax = sysconf(_SC_ARG_MAX);
    if(OSArgMax < 0)
@@ -1731,6 +1774,7 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 
    // this loop is runs once per dpkg operation
    vector<Item>::const_iterator I = List.cbegin();
+   BuildDpkgCall Args;
    while (I != List.end())
    {
       // Do all actions with the same Op in one run
@@ -1752,82 +1796,68 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
       else
 	 J = std::find_if(J, List.cend(), [&J](Item const &I) { return I.Op != J->Op; });
 
-      auto const size = (J - I) + 10;
-
-      // start with the baseset of arguments
-      auto Size = StartSize;
-      Args.erase(Args.begin() + BaseArgs, Args.end());
-      Args.reserve(size);
+      Args.clearCallArguments();
+      Args.reserve((J - I) + 10);
 
       int fd[2];
       if (pipe(fd) != 0)
 	 return _error->Errno("pipe","Failed to create IPC pipe to dpkg");
 
-      auto const AddArg = [&](auto const argument) {
-	 Args.push_back(argument);
-	 Size += strlen(argument);
-      };
-
-      AddArg("--status-fd");
-      char status_fd_buf[20];
-      snprintf(status_fd_buf,sizeof(status_fd_buf),"%i", fd[1]);
-      AddArg(status_fd_buf);
+      Args.push_back("--status-fd");
+      Args.push_back(std::to_string(fd[1]));
       unsigned long const Op = I->Op;
 
       if (NoTriggers == true && I->Op != Item::TriggersPending &&
 	  (I->Op != Item::ConfigurePending || std::next(I) != List.end()))
-	 AddArg("--no-triggers");
+	 Args.push_back("--no-triggers");
 
       switch (I->Op)
       {
 	 case Item::Remove:
 	 case Item::Purge:
-	 AddArg("--force-depends");
-	 AddArg("--abort-after=1");
+	 Args.push_back("--force-depends");
+	 Args.push_back("--abort-after=1");
 	 if (std::any_of(I, J, ItemIsEssential))
-	    AddArg("--force-remove-essential");
+	    Args.push_back("--force-remove-essential");
 	 if (dpkgProtectedField && std::any_of(I, J, ItemIsProtected))
-	    AddArg("--force-remove-protected");
-	 AddArg("--remove");
+	    Args.push_back("--force-remove-protected");
+	 Args.push_back("--remove");
 	 break;
 
 	 case Item::Configure:
-	 AddArg("--configure");
+	 Args.push_back("--configure");
 	 break;
 
 	 case Item::ConfigurePending:
-	 AddArg("--configure");
-	 AddArg("--pending");
+	 Args.push_back("--configure");
+	 Args.push_back("--pending");
 	 break;
 
 	 case Item::TriggersPending:
-	 AddArg("--triggers-only");
-	 AddArg("--pending");
+	 Args.push_back("--triggers-only");
+	 Args.push_back("--pending");
 	 break;
 
 	 case Item::RemovePending:
-	 AddArg("--remove");
-	 AddArg("--pending");
+	 Args.push_back("--remove");
+	 Args.push_back("--pending");
 	 break;
 
 	 case Item::PurgePending:
-	 AddArg("--purge");
-	 AddArg("--pending");
+	 Args.push_back("--purge");
+	 Args.push_back("--pending");
 	 break;
 
 	 case Item::Install:
-	 AddArg("--unpack");
-	 AddArg("--auto-deconfigure");
+	 Args.push_back("--unpack");
+	 Args.push_back("--auto-deconfigure");
 	 // dpkg < 1.20.8 needs --force-remove-protected to deconfigure protected packages
 	 if (dpkgProtectedField)
-	    AddArg("--force-remove-protected");
+	    Args.push_back("--force-remove-protected");
 	 break;
       }
 
       std::unique_ptr<char, decltype(&cleanUpTmpDir)> tmpdir_for_dpkg_recursive{nullptr, &cleanUpTmpDir};
-      // keep track of allocated strings for multiarch package names
-      std::vector<std::unique_ptr<char, decltype(std::free) *>> Packages;
-      Packages.reserve(size);
 
       // Write in the file or package names
       if (I->Op == Item::Install)
@@ -1858,17 +1888,16 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 	       if (symlink(I->File.c_str(), linkpath.c_str()) != 0)
 		  return _error->Errno("DPkg::Go", "Symlinking %s to %s failed!", I->File.c_str(), linkpath.c_str());
 	    }
-	    AddArg("--recursive");
-	    AddArg(tmpdir_for_dpkg_recursive.get());
+	    Args.push_back("--recursive");
+	    Args.push_back(tmpdir_for_dpkg_recursive.get());
 	 }
 	 else
 	 {
-	    for (;I != J && Size < MaxArgBytes; ++I)
+	    for (;I != J && Args.bytes() < MaxArgBytes; ++I)
 	    {
 	       if (I->File[0] != '/')
 		  return _error->Error("Internal Error, Pathname to install is not absolute '%s'",I->File.c_str());
 	       Args.push_back(I->File.c_str());
-	       Size += I->File.length();
 	    }
 	 }
       }
@@ -1902,8 +1931,8 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
       else
       {
 	 string const nativeArch = _config->Find("APT::Architecture");
-	 unsigned long const oldSize = I->Pkg.end() == false ? Size : 0;
-	 for (;I != J && Size < MaxArgBytes; ++I)
+	 auto const oldSize = I->Pkg.end() ? 0ull : Args.bytes();
+	 for (;I != J && Args.bytes() < MaxArgBytes; ++I)
 	 {
 	    if((*I).Pkg.end() == true)
 	       continue;
@@ -1913,34 +1942,29 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 	    if (dpkgMultiArch == false && (I->Pkg.Arch() == nativeArch ||
 					   strcmp(I->Pkg.Arch(), "all") == 0 ||
 					   strcmp(I->Pkg.Arch(), "none") == 0))
-	       AddArg(I->Pkg.Name());
+	       Args.push_back(I->Pkg.Name());
+	    else if (Op == Item::Purge && I->Pkg->CurrentVer == 0)
+	       continue; // we purge later with --purge --pending, so if it isn't installed (aka rc-only), skip it here
+	    else if (strcmp(I->Pkg.Arch(), "none") == 0)
+	       Args.push_back(I->Pkg.Name()); // never arch-qualify a package without an arch
 	    else
 	    {
 	       pkgCache::VerIterator PkgVer;
-	       std::string name = I->Pkg.Name();
-	       if (Op == Item::Remove)
-		  PkgVer = I->Pkg.CurrentVer();
-	       else if (Op == Item::Purge)
-	       {
-		  // we purge later with --purge --pending, so if it isn't installed (aka rc-only), skip it here
+	       if (Op == Item::Remove || Op == Item::Purge)
 		  PkgVer = I->Pkg.CurrentVer();
-		  if (PkgVer.end() == true)
-		     continue;
-	       }
 	       else
 		  PkgVer = Cache[I->Pkg].InstVerIter(Cache);
-	       if (strcmp(I->Pkg.Arch(), "none") == 0)
-		  ; // never arch-qualify a package without an arch
-	       else if (PkgVer.end() == false)
-		  name.append(":").append(PkgVer.Arch());
-	       else
-		  _error->Warning("Can not find PkgVer for '%s'", name.c_str());
-	       Packages.emplace_back(strdup(name.c_str()), &std::free);
-	       AddArg(Packages.back().get());
+	       if (PkgVer.end())
+	       {
+		  _error->Warning("Can not find PkgVer for '%s'", I->Pkg.Name());
+		  Args.push_back(I->Pkg.Name());
+		  continue;
+	       }
+	       Args.push_back(std::string(I->Pkg.Name()) + ":" + PkgVer.Arch());
 	    }
 	 }
 	 // skip configure action if all scheduled packages disappeared
-	 if (oldSize == Size)
+	 if (oldSize == Args.bytes())
 	    continue;
       }
 
@@ -1948,16 +1972,13 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 
       if (noopDPkgInvocation == true)
       {
-	 for (std::vector<const char *>::const_iterator a = Args.begin();
-	      a != Args.end(); ++a)
-	    clog << *a << ' ';
+	 for (auto const a : Args)
+	    clog << a << ' ';
 	 clog << endl;
 	 close(fd[0]);
 	 close(fd[1]);
 	 continue;
       }
-      Args.push_back(NULL);
-
       cout << flush;
       clog << flush;
       cerr << flush;
@@ -2023,9 +2044,7 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 	 if (_config->Find("DPkg::Path", "").empty() == false)
 	    setenv("PATH", _config->Find("DPkg::Path", "").c_str(), 1);
 
-	 execvp(Args[0], (char**) &Args[0]);
-	 cerr << "Could not exec dpkg!" << endl;
-	 _exit(100);
+	 Args.execute("Could not exec dpkg!");
       }
 
       // we read from dpkg here
@@ -2041,9 +2060,6 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
       sigemptyset(&d->sigmask);
       sigprocmask(SIG_BLOCK,&d->sigmask,&d->original_sigmask);
 
-      /* free vectors (and therefore memory) as we don't need the included data anymore */
-      Packages.clear();
-
       // the result of the waitpid call
       int Status = 0;
       int res;
@@ -2068,14 +2084,16 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 	    break;
 
 	 // wait for input or output here
+	 fd_set rfds;
 	 FD_ZERO(&rfds);
 	 if (d->master >= 0 && d->direct_stdin == false && d->stdin_is_dev_null == false)
 	    FD_SET(STDIN_FILENO, &rfds);
 	 FD_SET(_dpkgin, &rfds);
 	 if(d->master >= 0)
 	    FD_SET(d->master, &rfds);
-         tv.tv_sec = 0;
-         tv.tv_nsec = d->progress->GetPulseInterval();
+	 struct timespec tv;
+	 tv.tv_sec = 0;
+	 tv.tv_nsec = d->progress->GetPulseInterval();
 	 auto const select_ret = pselect(max(d->master, _dpkgin)+1, &rfds, NULL, NULL,
 			      &tv, &d->original_sigmask);
          d->progress->Pulse();
@@ -2106,7 +2124,7 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 
       if (waitpid_failure == true)
       {
-	 strprintf(d->dpkg_error, "Sub-process %s couldn't be waited for.",Args[0]);
+	 strprintf(d->dpkg_error, "Sub-process %s couldn't be waited for.",Args.front());
 	 _error->Error("%s", d->dpkg_error.c_str());
 	 break;
       }
@@ -2120,11 +2138,11 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
 	 bool const stopOnError = _config->FindB("Dpkg::StopOnError",true);
 
 	 if (WIFSIGNALED(Status) != 0 && WTERMSIG(Status) == SIGSEGV)
-	    strprintf(d->dpkg_error, "Sub-process %s received a segmentation fault.",Args[0]);
+	    strprintf(d->dpkg_error, "Sub-process %s received a segmentation fault.",Args.front());
 	 else if (WIFEXITED(Status) != 0)
-	    strprintf(d->dpkg_error, "Sub-process %s returned an error code (%u)",Args[0],WEXITSTATUS(Status));
+	    strprintf(d->dpkg_error, "Sub-process %s returned an error code (%u)",Args.front(),WEXITSTATUS(Status));
 	 else
-	    strprintf(d->dpkg_error, "Sub-process %s exited unexpectedly",Args[0]);
+	    strprintf(d->dpkg_error, "Sub-process %s exited unexpectedly",Args.front());
 	 _error->Error("%s", d->dpkg_error.c_str());
 
 	 if(stopOnError)