wrap every unlink call to check for != /dev/null

Unlinking /dev/null is bad, we shouldn't do that. Also, we should print
at least a warning if we tried to unlink a file but didn't manage to
pull it of (ignoring the case were the file is /dev/null or doesn't
exist in the first place).

This got triggered by a relatively unlikely to cause problem in
pkgAcquire::Worker::PrepareFiles which would while temporary
uncompressed files (which are set to keep compressed) figure out that to
files are the same and prepare for sharing by deleting them. Bad move.
That also shows why not printing a warning is a bad idea as this hide
the error for in non-root test runs.

Git-Dch: Ignore

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 6cf23daae1088e84f94b7b36ca3c460c920fe7c3..834776404e5cb9defbf774d331fb6b79292e8517 100644 (file)
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -147,20 +147,6 @@ static bool BootstrapPDiffWith(std::string const &PartialFile, std::string const
    return typeItr != types.cend();
 }
                                                                        /*}}}*/
-static bool RemoveFile(char const * const Function, std::string const &FileName)/*{{{*/
-{
-   if (FileName == "/dev/null")
-      return true;
-   errno = 0;
-   if (unlink(FileName.c_str()) != 0)
-   {
-      if (errno == ENOENT)
-        return true;
-      return _error->WarningE(Function, "Removal of file %s failed", FileName.c_str());
-   }
-   return true;
-}
-                                                                       /*}}}*/
 
 static bool MessageInsecureRepository(bool const isError, std::string const &msg)/*{{{*/
 {

diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc
index d8bdf56990fe8adae190d24cf1a3c4886ea2b243..b5f52a3caf6781aaa62f751a57f82de25b962dbd 100644 (file)
--- a/apt-pkg/acquire-worker.cc
+++ b/apt-pkg/acquire-worker.cc
@@ -742,9 +742,9 @@ void pkgAcquire::Worker::PrepareFiles(char const * const caller, pkgAcquire::Que
       for (pkgAcquire::Queue::QItem::owner_iterator O = Itm->Owners.begin(); O != Itm->Owners.end(); ++O)
       {
         pkgAcquire::Item const * const Owner = *O;
-        if (Owner->DestFile == filename)
+        if (Owner->DestFile == filename || filename == "/dev/null")
            continue;
-        unlink(Owner->DestFile.c_str());
+        RemoveFile("PrepareFiles", Owner->DestFile);
         if (link(filename.c_str(), Owner->DestFile.c_str()) != 0)
         {
            // different mounts can't happen for us as we download to lists/ by default,
@@ -759,7 +759,7 @@ void pkgAcquire::Worker::PrepareFiles(char const * const caller, pkgAcquire::Que
    else
    {
       for (pkgAcquire::Queue::QItem::owner_iterator O = Itm->Owners.begin(); O != Itm->Owners.end(); ++O)
-        unlink((*O)->DestFile.c_str());
+        RemoveFile("PrepareFiles", (*O)->DestFile);
    }
 }
                                                                        /*}}}*/

diff --git a/apt-pkg/acquire.cc b/apt-pkg/acquire.cc
index a74f1c2f648697cef952f91a70a580313d827d06..81faee5d8d07086dc2ba762fb689f56067354c20 100644 (file)
--- a/apt-pkg/acquire.cc
+++ b/apt-pkg/acquire.cc
@@ -645,7 +645,7 @@ bool pkgAcquire::Clean(string Dir)
       
       // Nothing found, nuke it
       if (I == Items.end())
-        unlink(Dir->d_name);
+        RemoveFile("Clean", Dir->d_name);
    };
    
    closedir(D);
@@ -993,15 +993,15 @@ void pkgAcquire::Queue::QItem::SyncDestinationFiles() const               /*{{{*/
       if (lstat((*O)->DestFile.c_str(),&file) == 0)
       {
         if ((file.st_mode & S_IFREG) == 0)
-           unlink((*O)->DestFile.c_str());
+           RemoveFile("SyncDestinationFiles", (*O)->DestFile);
         else if (supersize < file.st_size)
         {
            supersize = file.st_size;
-           unlink(superfile.c_str());
+           RemoveFile("SyncDestinationFiles", superfile);
            rename((*O)->DestFile.c_str(), superfile.c_str());
         }
         else
-           unlink((*O)->DestFile.c_str());
+           RemoveFile("SyncDestinationFiles", (*O)->DestFile);
         if (symlink(superfile.c_str(), (*O)->DestFile.c_str()) != 0)
         {
            ; // not a problem per-se and no real alternative

diff --git a/apt-pkg/cachefile.cc b/apt-pkg/cachefile.cc
index 1c9bc694b68e1e899afbbf4301f26687fe17657b..aaa2436c5d787fcf4bcad623b1f93afc37efb963 100644 (file)
--- a/apt-pkg/cachefile.cc
+++ b/apt-pkg/cachefile.cc
@@ -191,9 +191,9 @@ void pkgCacheFile::RemoveCaches()
    std::string const srcpkgcache = _config->FindFile("Dir::cache::srcpkgcache");
 
    if (pkgcache.empty() == false && RealFileExists(pkgcache) == true)
-      unlink(pkgcache.c_str());
+      RemoveFile("RemoveCaches", pkgcache);
    if (srcpkgcache.empty() == false && RealFileExists(srcpkgcache) == true)
-      unlink(srcpkgcache.c_str());
+      RemoveFile("RemoveCaches", srcpkgcache);
    if (pkgcache.empty() == false)
    {
       std::string cachedir = flNotFile(pkgcache);
@@ -207,7 +207,7 @@ void pkgCacheFile::RemoveCaches()
            std::string nuke = flNotDir(*file);
            if (strncmp(cachefile.c_str(), nuke.c_str(), cachefile.length()) != 0)
               continue;
-           unlink(file->c_str());
+           RemoveFile("RemoveCaches", *file);
         }
       }
    }
@@ -226,7 +226,7 @@ void pkgCacheFile::RemoveCaches()
       std::string nuke = flNotDir(*file);
       if (strncmp(cachefile.c_str(), nuke.c_str(), cachefile.length()) != 0)
         continue;
-      unlink(file->c_str());
+      RemoveFile("RemoveCaches", *file);
    }
 }
                                                                        /*}}}*/

diff --git a/apt-pkg/cdrom.cc b/apt-pkg/cdrom.cc
index b950648b908cfeab5b9817c70facbf92dd2ac67d..29eeca066a21e0120df49660a713efbb7a9d2421 100644 (file)
--- a/apt-pkg/cdrom.cc
+++ b/apt-pkg/cdrom.cc
@@ -426,8 +426,8 @@ bool pkgCdrom::WriteDatabase(Configuration &Cnf)
 {
    string DFile = _config->FindFile("Dir::State::cdroms");
    string NewFile = DFile + ".new";
-   
-   unlink(NewFile.c_str());
+
+   RemoveFile("WriteDatabase", NewFile);
    ofstream Out(NewFile.c_str());
    if (!Out)
       return _error->Errno("ofstream::ofstream",
@@ -468,7 +468,7 @@ bool pkgCdrom::WriteSourceList(string Name,vector<string> &List,bool Source)
       return _error->Errno("ifstream::ifstream","Opening %s",File.c_str());
 
    string NewFile = File + ".new";
-   unlink(NewFile.c_str());
+   RemoveFile("WriteDatabase", NewFile);
    ofstream Out(NewFile.c_str());
    if (!Out)
       return _error->Errno("ofstream::ofstream",

diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
index 368380af7817837f5c37a3ee8afa1c56be94e2ff..e52c8f219c5baa9442e7630ceab172b335895966 100644 (file)
--- a/apt-pkg/contrib/fileutl.cc
+++ b/apt-pkg/contrib/fileutl.cc
@@ -169,6 +169,21 @@ bool CopyFile(FileFd &From,FileFd &To)
         return false;
    } while (ToRead != 0);
 
+   return true;
+}
+                                                                       /*}}}*/
+bool RemoveFile(char const * const Function, std::string const &FileName)/*{{{*/
+{
+   if (FileName == "/dev/null")
+      return true;
+   errno = 0;
+   if (unlink(FileName.c_str()) != 0)
+   {
+      if (errno == ENOENT)
+        return true;
+
+      return _error->WarningE(Function,_("Problem unlinking the file %s"), FileName.c_str());
+   }
    return true;
 }
                                                                        /*}}}*/
@@ -1135,13 +1150,13 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co
    else if ((OpenMode & (Exclusive | Create)) == (Exclusive | Create))
    {
       // for atomic, this will be done by rename in Close()
-      unlink(FileName.c_str());
+      RemoveFile("FileFd::Open", FileName);
    }
    if ((OpenMode & Empty) == Empty)
    {
       struct stat Buf;
       if (lstat(FileName.c_str(),&Buf) == 0 && S_ISLNK(Buf.st_mode))
-        unlink(FileName.c_str());
+        RemoveFile("FileFd::Open", FileName);
    }
 
    int fileflags = 0;
@@ -2025,8 +2040,7 @@ bool FileFd::Close()
 
    if ((Flags & Fail) == Fail && (Flags & DelOnFail) == DelOnFail &&
        FileName.empty() == false)
-      if (unlink(FileName.c_str()) != 0)
-        Res &= _error->WarningE("unlnk",_("Problem unlinking the file %s"), FileName.c_str());
+      Res &= RemoveFile("FileFd::Close", FileName);
 
    if (Res == false)
       Flags |= Fail;

diff --git a/apt-pkg/contrib/fileutl.h b/apt-pkg/contrib/fileutl.h
index cddfe2b45cfa5c51c6a7c80aa0b9bdfe812231ac..7176e4dea03f923dc56351a430a269cb3bc6de0e 100644 (file)
--- a/apt-pkg/contrib/fileutl.h
+++ b/apt-pkg/contrib/fileutl.h
@@ -150,6 +150,7 @@ class FileFd
 
 bool RunScripts(const char *Cnf);
 bool CopyFile(FileFd &From,FileFd &To);
+bool RemoveFile(char const * const Function, std::string const &FileName);
 int GetLock(std::string File,bool Errors = true);
 bool FileExists(std::string File);
 bool RealFileExists(std::string File);

diff --git a/apt-pkg/deb/dpkgpm.cc b/apt-pkg/deb/dpkgpm.cc
index ccc4b5a6c07fac2cfda180f887790638ec0e0384..1446826d6056693d3122dc6c9dfa9cd165812f68 100644 (file)
--- a/apt-pkg/deb/dpkgpm.cc
+++ b/apt-pkg/deb/dpkgpm.cc
@@ -1597,7 +1597,7 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
    {
       std::string const oldpkgcache = _config->FindFile("Dir::cache::pkgcache");
       if (oldpkgcache.empty() == false && RealFileExists(oldpkgcache) == true &&
-         unlink(oldpkgcache.c_str()) == 0)
+         RemoveFile("pkgDPkgPM::Go", oldpkgcache))
       {
         std::string const srcpkgcache = _config->FindFile("Dir::cache::srcpkgcache");
         if (srcpkgcache.empty() == false && RealFileExists(srcpkgcache) == true)

diff --git a/apt-pkg/edsp/edsplistparser.cc b/apt-pkg/edsp/edsplistparser.cc
index 77a0edc22c8ef13d8e4112b68a2e90b969ef7f42..85a57479ec2e83807857adad51d1766a8699dff4 100644 (file)
--- a/apt-pkg/edsp/edsplistparser.cc
+++ b/apt-pkg/edsp/edsplistparser.cc
@@ -31,12 +31,10 @@ public:
    edspListParserPrivate()
    {
       std::string const states = _config->FindFile("Dir::State::extended_states");
-      if (states != "/dev/null")
-        unlink(states.c_str());
+      RemoveFile("edspListParserPrivate", states);
       extendedstates.Open(states, FileFd::WriteOnly | FileFd::Create | FileFd::Exclusive, 0600);
       std::string const prefs = _config->FindFile("Dir::Etc::preferences");
-      if (prefs != "/dev/null")
-        unlink(prefs.c_str());
+      RemoveFile("edspListParserPrivate", prefs);
       preferences.Open(prefs, FileFd::WriteOnly | FileFd::Create | FileFd::Exclusive, 0600);
    }
 };

diff --git a/apt-pkg/edsp/edspsystem.cc b/apt-pkg/edsp/edspsystem.cc
index 4c16f76d2f8c53cd10a3f77af574e50a5931d553..95abc15277e79d42a10c2e244a254a13bc271821 100644 (file)
--- a/apt-pkg/edsp/edspsystem.cc
+++ b/apt-pkg/edsp/edspsystem.cc
@@ -57,8 +57,8 @@ public:
       if (tempDir.empty())
         return;
 
-      unlink(tempStatesFile.c_str());
-      unlink(tempPrefsFile.c_str());
+      RemoveFile("DeInitialize", tempStatesFile);
+      RemoveFile("DeInitialize", tempPrefsFile);
       rmdir(tempDir.c_str());
    }
 

diff --git a/apt-private/private-download.cc b/apt-private/private-download.cc
index 4894c72bfe8f3767879f537145488bf2bd69c5ef..dcb604f2a526a7cc39d21922592ad7f9da34a944 100644 (file)
--- a/apt-private/private-download.cc
+++ b/apt-private/private-download.cc
@@ -331,7 +331,7 @@ bool DoClean(CommandLine &)
         c1out << "Del " << Pkg << " " << Ver << " [" << SizeToStr(St.st_size) << "B]" << std::endl;
 
         if (_config->FindB("APT::Get::Simulate") == false)
-           unlink(File);
+           RemoveFile("Cleaner::Erase", File);
       };
 };
 bool DoAutoClean(CommandLine &)

diff --git a/cmdline/apt-dump-solver.cc b/cmdline/apt-dump-solver.cc
index 47b515be5cb70abbd6c8ad63e7495bb8c1e94211..27592f22eb5f3e5258785f27cd9fd3300735f53d 100644 (file)
--- a/cmdline/apt-dump-solver.cc
+++ b/cmdline/apt-dump-solver.cc
@@ -53,8 +53,7 @@ int main(int argc,const char *argv[])                                 /*{{{*/
           return 0;
        }
 
-       if (strcmp(filename, "/dev/null") != 0)
-          unlink(filename);
+       RemoveFile(argv[0], filename);
        FileFd input, output;
        if (input.OpenDescriptor(STDIN_FILENO, FileFd::ReadOnly) == false ||
              output.Open(filename, FileFd::WriteOnly | FileFd::Create | FileFd::Exclusive, 0600) == false ||

diff --git a/ftparchive/byhash.cc b/ftparchive/byhash.cc
index 0a38457c0f657cd57c51ebeeb546936b92e16e12..354d089c31c339ade54785a820676f7be715e9c8 100644 (file)
--- a/ftparchive/byhash.cc
+++ b/ftparchive/byhash.cc
@@ -41,9 +41,8 @@ void DeleteAllButMostRecent(std::string dir, int KeepFiles)
    auto files = GetListOfFilesInDir(dir, false);
    std::sort(files.begin(), files.end(), Cmp());
 
-   for (auto I=files.begin(); I<files.end()-KeepFiles; I++) {
-      unlink((*I).c_str());
-   }
+   for (auto I=files.begin(); I<files.end()-KeepFiles; ++I)
+      RemoveFile("DeleteAllButMostRecent", *I);
 }
 
 // Takes a input filename (e.g. binary-i386/Packages) and a hashstring

diff --git a/ftparchive/multicompress.cc b/ftparchive/multicompress.cc
index 08a3cff5a97abc3342e35b1bbba4f9c417e05099..3ffc5266ee22f8d9547fa7465017b52d20c00b0f 100644 (file)
--- a/ftparchive/multicompress.cc
+++ b/ftparchive/multicompress.cc
@@ -352,9 +352,7 @@ bool MultiCompress::Child(int const &FD)
         for (Files *I = Outputs; I != 0; I = I->Next)
         {
            I->TmpFile.Close();
-           if (unlink(I->TmpFile.Name().c_str()) != 0)
-              _error->Errno("unlink",_("Problem unlinking %s"),
-                            I->TmpFile.Name().c_str());
+           RemoveFile("MultiCompress::Child", I->TmpFile.Name());
         }
         return !_error->PendingError();
       }      

diff --git a/ftparchive/writer.cc b/ftparchive/writer.cc
index 0bd2be566fa8a59916a202d6967b5f6bbbc20f6a..c0223a74cd3371375d2faf579e24b1b93c645a50 100644 (file)
--- a/ftparchive/writer.cc
+++ b/ftparchive/writer.cc
@@ -302,9 +302,7 @@ bool FTWScanner::Delink(string &FileName,const char *OriginalPath,
               _error->Errno("readlink",_("Failed to readlink %s"),OriginalPath);
            else
            {
-              if (unlink(OriginalPath) != 0)
-                 _error->Errno("unlink",_("Failed to unlink %s"),OriginalPath);
-              else
+              if (RemoveFile("FTWScanner::Delink", OriginalPath))
               {
                  if (link(FileName.c_str(),OriginalPath) != 0)
                  {

diff --git a/methods/file.cc b/methods/file.cc
index b689de6195c543276987fe7d84ca9e90209bb71a..8a087c36dc8b24bc9c9692b6db46cddf1929c66b 100644 (file)
--- a/methods/file.cc
+++ b/methods/file.cc
@@ -76,7 +76,7 @@ bool FileMethod::Fetch(FetchItem *Itm)
       }
    }
    if (Res.IMSHit != true)
-      unlink(Itm->DestFile.c_str());
+      RemoveFile("file", Itm->DestFile);
 
    // See if the file exists
    if (stat(File.c_str(),&Buf) == 0)

diff --git a/methods/ftp.cc b/methods/ftp.cc
index d84a194ca3b4c3b72bed6d654cdf45ceeb6a68e5..360333107bdeac352181dc241ce53169baef17fc 100644 (file)
--- a/methods/ftp.cc
+++ b/methods/ftp.cc
@@ -1090,7 +1090,7 @@ bool FtpMethod::Fetch(FetchItem *Itm)
         // If the file is missing we hard fail and delete the destfile
         // otherwise transient fail
         if (Missing == true) {
-           unlink(FailFile.c_str());
+           RemoveFile("ftp", FailFile);
            return false;
         }
         Fail(true);

diff --git a/methods/https.cc b/methods/https.cc
index a159159105c8882136afe8d95e48b25dd2727e5a..8d945454525b5b3848a68b4db5568d4375c4277c 100644 (file)
--- a/methods/https.cc
+++ b/methods/https.cc
@@ -443,7 +443,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm)
    // server says file not modified
    if (Server->Result == 304 || curl_condition_unmet == 1)
    {
-      unlink(File->Name().c_str());
+      RemoveFile("https", File->Name());
       Res.IMSHit = true;
       Res.LastModified = Itm->LastModified;
       Res.Size = 0;
@@ -461,14 +461,14 @@ bool HttpsMethod::Fetch(FetchItem *Itm)
       SetFailReason(err);
       _error->Error("%i %s", Server->Result, Server->Code);
       // unlink, no need keep 401/404 page content in partial/
-      unlink(File->Name().c_str());
+      RemoveFile("https", File->Name());
       return false;
    }
 
    // invalid range-request
    if (Server->Result == 416)
    {
-      unlink(File->Name().c_str());
+      RemoveFile("https", File->Name());
       delete File;
       Redirect(Itm->Uri);
       return true;

diff --git a/methods/mirror.cc b/methods/mirror.cc
index d3aef91bcd3d68054a96754152f418b463d09d8f..56362d317a9dfc499f56f3fa5803456fbfbbe197 100644 (file)
--- a/methods/mirror.cc
+++ b/methods/mirror.cc
@@ -122,7 +122,7 @@ bool MirrorMethod::Clean(string Dir)
       }
       // nothing found, nuke it
       if (I == list.end())
-        unlink(Dir->d_name);
+        RemoveFile("mirror", Dir->d_name);
    }
 
    closedir(D);

diff --git a/methods/server.cc b/methods/server.cc
index 934ec2abe6361c04192a31b0f26b7011d19b610f..650a4aeb8e3d19f1e92885ed2a99349a9114be8c 100644 (file)
--- a/methods/server.cc
+++ b/methods/server.cc
@@ -274,7 +274,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res)
    // Not Modified
    if (Server->Result == 304)
    {
-      unlink(Queue->DestFile.c_str());
+      RemoveFile("server", Queue->DestFile);
       Res.IMSHit = true;
       Res.LastModified = Queue->LastModified;
       return IMS_HIT;
@@ -350,7 +350,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res)
            Server->StartPos = Server->TotalFileSize;
            Server->Result = 200;
         }
-        else if (unlink(Queue->DestFile.c_str()) == 0)
+        else if (RemoveFile("server", Queue->DestFile))
         {
            NextURI = Queue->Uri;
            return TRY_AGAIN_OR_REDIRECT;