Do not read stderr from proxy autodetection scripts

This fixes a regression introduced in
  commit 8f858d560e3b7b475c623c4e242d1edce246025a

  don't leak FD in AutoProxyDetect command return parsing

which accidentally made the proxy autodetection code also read
the scripts output on stderr, not only on stdout when it switched
the code from popen() to Popen().

Reported-By: Tim Small <tim@seoss.co.uk>
(cherry picked from commit 0ecceb5bb9cc8727c117195945b7116aceb984fe)

diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
index 20ccaa1c6d19eb87be52b93be2d3ed671b9415f5..df39ae16004866f72cffaa33ea09d3bd2e9f71fc 100644 (file)
--- a/apt-pkg/contrib/fileutl.cc
+++ b/apt-pkg/contrib/fileutl.cc
@@ -2803,6 +2803,11 @@ bool Rename(std::string From, std::string To)                            /*{{{*/
 }
                                                                        /*}}}*/
 bool Popen(const char* Args[], FileFd &Fd, pid_t &Child, FileFd::OpenMode Mode)/*{{{*/
+{
+   return Popen(Args, Fd, Child, Mode, true);
+}
+                                                                       /*}}}*/
+bool Popen(const char* Args[], FileFd &Fd, pid_t &Child, FileFd::OpenMode Mode, bool CaptureStderr)/*{{{*/
 {
    int fd;
    if (Mode != FileFd::ReadOnly && Mode != FileFd::WriteOnly)
@@ -2834,7 +2839,8 @@ bool Popen(const char* Args[], FileFd &Fd, pid_t &Child, FileFd::OpenMode Mode)/
       if(Mode == FileFd::ReadOnly)
       {
          dup2(fd, 1);
-         dup2(fd, 2);
+        if (CaptureStderr == true)
+           dup2(fd, 2);
       } else if(Mode == FileFd::WriteOnly)
          dup2(fd, 0);
 

diff --git a/apt-pkg/contrib/fileutl.h b/apt-pkg/contrib/fileutl.h
index 9333b8d5fa6a295a593fd4721c58dbb8a8c9593f..7ee302d7bd2c0d23d5e4701e5a84d934e60bdbf4 100644 (file)
--- a/apt-pkg/contrib/fileutl.h
+++ b/apt-pkg/contrib/fileutl.h
@@ -245,8 +245,11 @@ std::vector<std::string> Glob(std::string const &pattern, int flags=0);
  * \param Child a reference to the integer that stores the child pid
  *        Note that you must call ExecWait() or similar to cleanup
  * \param Mode is either FileFd::ReadOnly or FileFd::WriteOnly
+ * \param CaptureStderr True if we should capture stderr in addition to stdout.
+ *                      (default: True).
  * \return true on success, false on failure with _error set
  */
+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);
 
 

diff --git a/apt-pkg/contrib/proxy.cc b/apt-pkg/contrib/proxy.cc
index 84d802dcb90834f4d506c69cd63997899438152d..13a6ab9405441966204b673309b10789b2d3f3a5 100644 (file)
--- a/apt-pkg/contrib/proxy.cc
+++ b/apt-pkg/contrib/proxy.cc
@@ -48,7 +48,7 @@ bool AutoDetectProxy(URI &URL)
    Args.push_back(nullptr);
    FileFd PipeFd;
    pid_t Child;
-   if(Popen(&Args[0], PipeFd, Child, FileFd::ReadOnly) == false)
+   if(Popen(&Args[0], PipeFd, Child, FileFd::ReadOnly, false) == false)
       return _error->Error("ProxyAutoDetect command '%s' failed!", AutoDetectProxyCmd.c_str());
    char buf[512];
    if (PipeFd.ReadLine(buf, sizeof(buf)) == nullptr)

diff --git a/test/libapt/apt-proxy-script b/test/libapt/apt-proxy-script
new file mode 100755 (executable)
index 0000000..41cfdc3
--- /dev/null
+++ b/test/libapt/apt-proxy-script
@@ -0,0 +1,9 @@
+#!/bin/sh
+
+if [ $1 = "http://www.debian.org:90/temp/test" ]; then
+    echo "http://example.com"
+fi
+if [ $1 = "http://www.debian.org:91/temp/test" ]; then
+    echo "This works" >&2
+    echo "http://example.com/foo"
+fi

diff --git a/test/libapt/uri_test.cc b/test/libapt/uri_test.cc
index 8296ca6a0de032ff1adaacf5835be541f8ff391f..09d0180493429547548f1841d752c63b167b2ace 100644 (file)
--- a/test/libapt/uri_test.cc
+++ b/test/libapt/uri_test.cc
@@ -1,4 +1,6 @@
 #include <config.h>
+#include <apt-pkg/configuration.h>
+#include <apt-pkg/proxy.h>
 #include <apt-pkg/strutl.h>
 #include <string>
 #include <gtest/gtest.h>
@@ -188,3 +190,27 @@ TEST(URITest, RFC2732)
    EXPECT_EQ("ftp://example.org", URI::ArchiveOnly(U));
    EXPECT_EQ("ftp://example.org/", URI::NoUserPassword(U));
 }
+TEST(URITest, AutoProxyTest)
+{
+   URI u0("http://www.debian.org:90/temp/test");
+   URI u1("http://www.debian.org:91/temp/test");
+
+   _config->Set("Acquire::http::Proxy-Auto-Detect", "./apt-proxy-script");
+
+   // Scenario 0: Autodetecting a simple proxy
+   AutoDetectProxy(u0);
+   EXPECT_EQ(_config->Find("Acquire::http::proxy::www.debian.org", ""), "http://example.com");
+
+   // Scenario 1: Proxy stays the same if it is already set
+   AutoDetectProxy(u1);
+   EXPECT_EQ(_config->Find("Acquire::http::proxy::www.debian.org", ""), "http://example.com");
+
+   // Scenario 2: Reading with stderr output works fine
+   _config->Clear("Acquire::http::proxy::www.debian.org");
+   AutoDetectProxy(u1);
+   EXPECT_EQ(_config->Find("Acquire::http::proxy::www.debian.org", ""), "http://example.com/foo");
+
+   // Scenario 1 again: Proxy stays the same if it is already set
+   AutoDetectProxy(u0);
+   EXPECT_EQ(_config->Find("Acquire::http::proxy::www.debian.org", ""), "http://example.com/foo");
+}