Remove nonfunctional and crashy pacrunner caching

libproxy currently attempts to cache pacrunner objects in the pr member
variable of its pacrunner_extension object. This is broken, though,
because it relies on the pacrunner object also being stored in
pacrunner_extension's last member variable, which is never written to.
So that caching has never worked properly. In practice, it only does one
thing: it causes a threadsafety bug, #68, because it causes the old
pacrunner object to be deleted on the thread that is creating the new
pacrunner, which is illegal for both the mozjs and WebKit-based
pacrunner extensions that expect their objects to be deleted on the same
thread they were created on.

This patch was originally written by Dan Winship for Fedora 19. It got
dropped in Fedora 24, then resurrected again for Fedora 28 after we
noticed 30,000 crash reports. I've tweaked it a bit to completely
remove the unused member variables.

Finally, note that this code is not exception-safe: if an exception is
thrown, the pacrunner could be leaked. But this seems to be a common
problem throughout libproxy. It should be fixed by using std::unique_ptr
instead of raw new and delete.

https://bugzilla.redhat.com/show_bug.cgi?id=998232

Fixes #68

3 files changed, 4 insertions, 14 deletions

diff --git a/libproxy/extension_pacrunner.cpp b/libproxy/extension_pacrunner.cpp
index de2ea7b..698f25f 100644
--- a/libproxy/extension_pacrunner.cpp
+++ b/libproxy/extension_pacrunner.cpp
@@ -23,19 +23,11 @@ using namespace libproxy;
 pacrunner::pacrunner(const string &, const url&) {}
 
 pacrunner_extension::pacrunner_extension() {
-	this->pr = NULL;
 }
 
 pacrunner_extension::~pacrunner_extension() {
-	if (this->pr) delete this->pr;
 }
 
 pacrunner* pacrunner_extension::get(const string &pac, const url& pacurl) {
-	if (this->pr) {
-		if (this->last == pac)
-			return this->pr;
-		delete this->pr;
-	}
-
-	return this->pr = this->create(pac, pacurl);
+	return this->create(pac, pacurl);
 }
diff --git a/libproxy/extension_pacrunner.hpp b/libproxy/extension_pacrunner.hpp
index eeb30f9..4834eb9 100644
--- a/libproxy/extension_pacrunner.hpp
+++ b/libproxy/extension_pacrunner.hpp
@@ -56,10 +56,6 @@ public:
 protected:
 	// Abstract methods
 	virtual pacrunner* create(string pac, const url& pacurl) =0;
-
-private:
-	pacrunner* pr;
-	string     last;
 };
 
 }
diff --git a/libproxy/proxy.cpp b/libproxy/proxy.cpp
index 2d01d53..af09e72 100644
--- a/libproxy/proxy.cpp
+++ b/libproxy/proxy.cpp
@@ -416,7 +416,9 @@ void proxy_factory::run_pac(url &realurl, const url &confurl, vector<string> &re
 
 		/* Run the PAC, but only try one PACRunner */
 		if (debug) cerr << "Using pacrunner: " << typeid(*pacrunners[0]).name() << endl;
-		string pacresp = pacrunners[0]->get(this->pac, this->pacurl->to_string())->run(realurl);
+		pacrunner* runner = pacrunners[0]->get(this->pac, this->pacurl->to_string());
+		string pacresp = runner->run(realurl);
+		delete runner;
 		if (debug) cerr << "Pacrunner returned: " << pacresp << endl;
 		format_pac_response(pacresp, response);
 	}