use FileFd to parse all apt configuration files

Using different ways of opening files means we have different behaviour
and error messages for them, so by the same for all we can have more
uniformity for users and apt developers alike.

8 files changed, 123 insertions, 32 deletions

diff --git a/apt-pkg/contrib/configuration.cc b/apt-pkg/contrib/configuration.cc
index 442e31dff..65242bec5 100644
--- a/apt-pkg/contrib/configuration.cc
+++ b/apt-pkg/contrib/configuration.cc
@@ -840,9 +840,9 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio
 		    unsigned const &Depth)
 {
    // Open the stream for reading
-   ifstream F(FName.c_str(),ios::in);
-   if (F.fail() == true)
-      return _error->Errno("ifstream::ifstream",_("Opening configuration file %s"),FName.c_str());
+   FileFd F;
+   if (OpenConfigurationFileFd(FName, F) == false)
+      return false;
 
    string LineBuffer;
    std::stack<std::string> Stack;
@@ -852,26 +852,15 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio
 
    int CurLine = 0;
    bool InComment = false;
-   while (F.eof() == false)
+   while (F.Eof() == false)
    {
       // The raw input line.
       std::string Input;
+      if (F.ReadLine(Input) == false)
+	 Input.clear();
       // The input line with comments stripped.
       std::string Fragment;
 
-      // Grab the next line of F and place it in Input.
-      do
-	{
-	  char *Buffer = new char[1024];
-
-	  F.clear();
-	  F.getline(Buffer,sizeof(Buffer) / 2);
-
-	  Input += Buffer;
-	  delete[] Buffer;
-	}
-      while (F.fail() && !F.eof());
-
       // Expand tabs in the input line and remove leading and trailing
       // whitespace.
       {
diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
index 630a98ce4..7633b07ef 100644
--- a/apt-pkg/contrib/fileutl.cc
+++ b/apt-pkg/contrib/fileutl.cc
@@ -2493,9 +2493,6 @@ bool FileFd::Read(int const Fd, void *To, unsigned long long Size, unsigned long
 }
 									/*}}}*/
 // FileFd::ReadLine - Read a complete line from the file		/*{{{*/
-// ---------------------------------------------------------------------
-/* Beware: This method can be quite slow for big buffers on UNcompressed
-   files because of the naive implementation! */
 char* FileFd::ReadLine(char *To, unsigned long long const Size)
 {
    *To = '\0';
@@ -2503,6 +2500,29 @@ char* FileFd::ReadLine(char *To, unsigned long long const Size)
       return nullptr;
    return d->InternalReadLine(To, Size);
 }
+bool FileFd::ReadLine(std::string &To)
+{
+   To.clear();
+   if (d == nullptr || Failed())
+      return false;
+   constexpr size_t buflen = 4096;
+   char buffer[buflen];
+   size_t len;
+   do
+   {
+      if (d->InternalReadLine(buffer, buflen) == nullptr)
+	 return false;
+      len = strlen(buffer);
+      To.append(buffer, len);
+   } while (len == buflen - 1 && buffer[len - 2] != '\n');
+   // remove the newline at the end
+   auto const i = To.find_last_not_of("\r\n");
+   if (i == std::string::npos)
+      To.clear();
+   else
+      To.erase(i + 1);
+   return true;
+}
 									/*}}}*/
 // FileFd::Flush - Flush the file  					/*{{{*/
 bool FileFd::Flush()
@@ -3104,3 +3124,9 @@ bool DropPrivileges()							/*{{{*/
    return true;
 }
 									/*}}}*/
+bool OpenConfigurationFileFd(std::string const &File, FileFd &Fd) /*{{{*/
+{
+   APT::Configuration::Compressor none(".", "", "", nullptr, nullptr, 0);
+   return Fd.Open(File, FileFd::ReadOnly, none);
+}
+									/*}}}*/
diff --git a/apt-pkg/contrib/fileutl.h b/apt-pkg/contrib/fileutl.h
index 5e857b5c8..06c303809 100644
--- a/apt-pkg/contrib/fileutl.h
+++ b/apt-pkg/contrib/fileutl.h
@@ -87,7 +87,28 @@ class FileFd
    }   
    bool Read(void *To,unsigned long long Size,unsigned long long *Actual = 0);
    bool static Read(int const Fd, void *To, unsigned long long Size, unsigned long long * const Actual = 0);
+   /** read a complete line or until buffer is full
+    *
+    * The buffer will always be \\0 terminated, so at most Size-1 characters are read.
+    * If the buffer holds a complete line the last character (before \\0) will be
+    * the newline character \\n otherwise the line was longer than the buffer.
+    *
+    * @param To buffer which will hold the line
+    * @param Size of the buffer to fill
+    * @param \b nullptr is returned in error cases, otherwise
+    * the parameter \b To now filled with the line.
+    */
    char* ReadLine(char *To, unsigned long long const Size);
+   /** read a complete line from the file
+    *
+    *  Similar to std::getline() the string does \b not include
+    *  the newline, but just the content of the line as the newline
+    *  is not needed to distinguish cases as for the other #ReadLine method.
+    *
+    *  @param To string which will hold the line
+    *  @return \b true if successful, otherwise \b false
+    */
+   bool ReadLine(std::string &To);
    bool Flush();
    bool Write(const void *From,unsigned long long Size);
    bool static Write(int Fd, const void *From, unsigned long long Size);
@@ -256,5 +277,6 @@ std::vector<std::string> Glob(std::string const &pattern, int flags=0);
 bool Popen(const char* Args[], FileFd &Fd, pid_t &Child, FileFd::OpenMode Mode, bool CaptureStderr);
 bool Popen(const char* Args[], FileFd &Fd, pid_t &Child, FileFd::OpenMode Mode);
 
+APT_HIDDEN bool OpenConfigurationFileFd(std::string const &File, FileFd &Fd);
 
 #endif
diff --git a/apt-pkg/policy.cc b/apt-pkg/policy.cc
index 008c98ecb..69c1fbe10 100644
--- a/apt-pkg/policy.cc
+++ b/apt-pkg/policy.cc
@@ -343,8 +343,11 @@ bool ReadPinFile(pkgPolicy &Plcy,string File)
 
    if (RealFileExists(File) == false)
       return true;
-   
-   FileFd Fd(File,FileFd::ReadOnly);
+
+   FileFd Fd;
+   if (OpenConfigurationFileFd(File, Fd) == false)
+      return false;
+
    pkgTagFile TF(&Fd, pkgTagFile::SUPPORT_COMMENTS);
    if (Fd.IsOpen() == false || Fd.Failed())
       return false;
diff --git a/apt-pkg/sourcelist.cc b/apt-pkg/sourcelist.cc
index 17c5c7a11..9f0c7f8ae 100644
--- a/apt-pkg/sourcelist.cc
+++ b/apt-pkg/sourcelist.cc
@@ -368,13 +368,12 @@ bool pkgSourceList::ReadAppend(string const &File)
 /* */
 bool pkgSourceList::ParseFileOldStyle(std::string const &File)
 {
-   // Open the stream for reading
-   ifstream F(File.c_str(),ios::in /*| ios::nocreate*/);
-   if (F.fail() == true)
-      return _error->Errno("ifstream::ifstream",_("Opening %s"),File.c_str());
+   FileFd Fd;
+   if (OpenConfigurationFileFd(File, Fd) == false)
+      return false;
 
    std::string Buffer;
-   for (unsigned int CurLine = 1; std::getline(F, Buffer); ++CurLine)
+   for (unsigned int CurLine = 1; Fd.ReadLine(Buffer); ++CurLine)
    {
       // remove comments
       size_t curpos = 0;
@@ -423,7 +422,9 @@ bool pkgSourceList::ParseFileOldStyle(std::string const &File)
 bool pkgSourceList::ParseFileDeb822(string const &File)
 {
    // see if we can read the file
-   FileFd Fd(File, FileFd::ReadOnly);
+   FileFd Fd;
+   if (OpenConfigurationFileFd(File, Fd) == false)
+      return false;
    pkgTagFile Sources(&Fd, pkgTagFile::SUPPORT_COMMENTS);
    if (Fd.IsOpen() == false || Fd.Failed())
       return _error->Error(_("Malformed stanza %u in source list %s (type)"),0,File.c_str());
diff --git a/test/integration/test-bug-818628-unreadable-source b/test/integration/test-bug-818628-unreadable-source
index cddc79398..0c781c3b9 100755
--- a/test/integration/test-bug-818628-unreadable-source
+++ b/test/integration/test-bug-818628-unreadable-source
@@ -54,13 +54,13 @@ echo 'Apt::Cmd::Disable-Script-Warning "true";' >> aptconfig.conf
 
 msgmsg 'Unreadable sources file'
 chmod -r rootdir/etc/apt/sources.list.d/apt-test-unstable-deb-src.list
-runthemall "E: Opening $TMPWORKINGDIRECTORY/rootdir/etc/apt/sources.list.d/apt-test-unstable-deb-src.list - ifstream::ifstream (13: Permission denied)
+runthemall "E: Could not open file $TMPWORKINGDIRECTORY/rootdir/etc/apt/sources.list.d/apt-test-unstable-deb-src.list - open (13: Permission denied)
 E: The list of sources could not be read."
 chmod +r rootdir/etc/apt/sources.list.d/apt-test-unstable-deb-src.list
 
 msgmsg 'Unreadable config file'
 chmod -r rootdir/etc/apt/apt.conf.d/unreadable.conf
-runthemall "E: Opening configuration file ${TMPWORKINGDIRECTORY}/rootdir/etc/apt/apt.conf.d/unreadable.conf - ifstream::ifstream (13: Permission denied)"
+runthemall "E: Could not open file ${TMPWORKINGDIRECTORY}/rootdir/etc/apt/apt.conf.d/unreadable.conf - open (13: Permission denied)"
 chmod +r rootdir/etc/apt/apt.conf.d/unreadable.conf
 
 msgmsg 'Unreadable preferences file'
diff --git a/test/libapt/configuration_test.cc b/test/libapt/configuration_test.cc
index bdc17cbf4..bfb7144ce 100644
--- a/test/libapt/configuration_test.cc
+++ b/test/libapt/configuration_test.cc
@@ -1,14 +1,14 @@
 #include <config.h>
 
 #include <apt-pkg/configuration.h>
+#include <apt-pkg/fileutl.h>
 
 #include <string>
 #include <vector>
 
 #include <gtest/gtest.h>
 
-//FIXME: Test for configuration file parsing;
-// currently only integration/ tests test them implicitly
+#include "file-helpers.h"
 
 TEST(ConfigurationTest,Lists)
 {
@@ -195,3 +195,40 @@ TEST(ConfigurationTest,Merge)
 	EXPECT_EQ("bar", Cnf.Find("option::foo"));
 	EXPECT_EQ("", Cnf.Find("option::empty"));
 }
+TEST(ConfigurationTest, Parsing)
+{
+   Configuration Cnf;
+   std::string tempfile;
+   FileFd fd;
+   createTemporaryFile("doublesignedfile", fd, &tempfile, R"apt(
+SimpleOption "true";
+/* SimpleOption "false"; */
+Answer::Simple "42";
+# This is a comment
+List::Option { "In"; "One"; "Line"; };
+// this a comment as well
+List::Option2 { "Multi";
+"Line"; # inline comment
+	 "Options";
+}; Trailing "true";
+/* Commented::Out "true"; */
+)apt");
+   EXPECT_TRUE(ReadConfigFile(Cnf, tempfile));
+   if (tempfile.empty() == false)
+      unlink(tempfile.c_str());
+   EXPECT_TRUE(Cnf.FindB("SimpleOption"));
+   EXPECT_EQ(42, Cnf.FindI("Answer::Simple"));
+   EXPECT_TRUE(Cnf.Exists("List::Option"));
+   auto const singleline = Cnf.FindVector("List::Option");
+   EXPECT_EQ(3, singleline.size());
+   EXPECT_EQ("In", singleline[0]);
+   EXPECT_EQ("One", singleline[1]);
+   EXPECT_EQ("Line", singleline[2]);
+   auto const multiline = Cnf.FindVector("List::Option2");
+   EXPECT_EQ(3, multiline.size());
+   EXPECT_EQ("Multi", multiline[0]);
+   EXPECT_EQ("Line", multiline[1]);
+   EXPECT_EQ("Options", multiline[2]);
+   EXPECT_TRUE(Cnf.FindB("Trailing"));
+   EXPECT_FALSE(Cnf.Exists("Commented::Out"));
+}
diff --git a/test/libapt/fileutl_test.cc b/test/libapt/fileutl_test.cc
index 6cc850033..a702c16ec 100644
--- a/test/libapt/fileutl_test.cc
+++ b/test/libapt/fileutl_test.cc
@@ -152,6 +152,19 @@ static void TestFileFd(mode_t const a_umask, mode_t const ExpectedFilePermission
       EXPECT_EQ(strlen(expect), f.Tell());
    }
 #undef APT_INIT_READBACK
+   {
+      test.erase(test.length() - 1);
+      EXPECT_TRUE(f.Seek(0));
+      EXPECT_FALSE(f.Eof());
+      std::string line;
+      EXPECT_TRUE(f.ReadLine(line));
+      EXPECT_FALSE(f.Failed());
+      EXPECT_FALSE(f.Eof());
+      EXPECT_EQ(line.length(), test.length());
+      EXPECT_EQ(line.length() + 1, f.Tell());
+      EXPECT_EQ(f.Size(), f.Tell());
+      EXPECT_EQ(line, test);
+   }
 
    f.Close();
    EXPECT_FALSE(f.IsOpen());