From 0ecceb5bb9cc8727c117195945b7116aceb984fe Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Sun, 2 Oct 2016 17:20:33 +0200 Subject: [PATCH] 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 --- apt-pkg/contrib/fileutl.cc | 8 +++++++- apt-pkg/contrib/fileutl.h | 3 +++ apt-pkg/contrib/proxy.cc | 2 +- test/libapt/apt-proxy-script | 9 +++++++++ test/libapt/uri_test.cc | 26 ++++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 2 deletions(-) create mode 100755 test/libapt/apt-proxy-script diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index fd13b45dc..6c43bed90 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -2860,6 +2860,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) @@ -2891,7 +2896,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 15665f8b5..dddeb70f5 100644 --- a/apt-pkg/contrib/fileutl.h +++ b/apt-pkg/contrib/fileutl.h @@ -248,8 +248,11 @@ std::vector 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 4529cf230..62cfba032 100644 --- 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]; bool const goodread = PipeFd.ReadLine(buf, sizeof(buf)) != nullptr; diff --git a/test/libapt/apt-proxy-script b/test/libapt/apt-proxy-script new file mode 100755 index 000000000..41cfdc30f --- /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 8296ca6a0..09d018049 100644 --- a/test/libapt/uri_test.cc +++ b/test/libapt/uri_test.cc @@ -1,4 +1,6 @@ #include +#include +#include #include #include #include @@ -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"); +} -- 2.45.2