Fix memory leak on HttpFetcher and ProxyResolver.
The current HttpFetcher and ProxyResolver code uses
google::protobuf::Closure callbacks created with NewCallback. These
callbacks will self-delete them when you call Run(), leaking the
callback if that doesn't happens.
This patch replaces all the NewCallback() calls by
NewPermanentCallback(), which won't delete the callback after running
it. It then adds a new utils::GlibDestroyClosure() function to use in
conjunction with the existing utils::GlibRunClosure() to schedule
callbacks from the glib main loop without leaking them.
Finally, this patch fixes a use-after-free on the
AbortingHttpFetcherTestDelegate class only affecting unit tests.
Other minor linting errors fixed.
BUG=chromium:378548
TEST=`FEATURES="test" USE="clang asan" emerge-link update_engine` doesn't complain about HttpFetcher.
Change-Id: Ica3265aca42f07811b7dff6131f9a43ab06269aa
Reviewed-on: https://chromium-review.googlesource.com/202062
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
diff --git a/chrome_browser_proxy_resolver.cc b/chrome_browser_proxy_resolver.cc
index 94b9dae..4f7b8bd 100644
--- a/chrome_browser_proxy_resolver.cc
+++ b/chrome_browser_proxy_resolver.cc
@@ -7,10 +7,10 @@
#include <map>
#include <string>
-#include <base/strings/string_util.h>
#include <base/strings/string_tokenizer.h>
-#include <dbus/dbus-glib.h>
+#include <base/strings/string_util.h>
#include <dbus/dbus-glib-lowlevel.h>
+#include <dbus/dbus-glib.h>
#include <google/protobuf/stubs/common.h>
#include "update_engine/dbus_constants.h"
@@ -20,7 +20,7 @@
using base::StringTokenizer;
using google::protobuf::Closure;
-using google::protobuf::NewCallback;
+using google::protobuf::NewPermanentCallback;
using std::deque;
using std::make_pair;
using std::multimap;
@@ -139,11 +139,13 @@
}
callbacks_.insert(make_pair(url, make_pair(callback, data)));
- Closure* closure = NewCallback(this,
- &ChromeBrowserProxyResolver::HandleTimeout,
- url);
+ Closure* closure = NewPermanentCallback(
+ this,
+ &ChromeBrowserProxyResolver::HandleTimeout,
+ url);
GSource* timer = g_timeout_source_new_seconds(timeout);
- g_source_set_callback(timer, &utils::GlibRunClosure, closure, NULL);
+ g_source_set_callback(
+ timer, utils::GlibRunClosure, closure, utils::GlibDestroyClosure);
g_source_attach(timer, NULL);
timers_.insert(make_pair(url, timer));
return true;
diff --git a/http_fetcher.cc b/http_fetcher.cc
index b59f988..2dd6ec4 100644
--- a/http_fetcher.cc
+++ b/http_fetcher.cc
@@ -34,7 +34,11 @@
bool HttpFetcher::ResolveProxiesForUrl(const string& url, Closure* callback) {
if (!proxy_resolver_) {
LOG(INFO) << "Not resolving proxies (no proxy resolver).";
- no_resolver_idle_id_ = g_idle_add(utils::GlibRunClosure, callback);
+ no_resolver_idle_id_ = g_idle_add_full(
+ G_PRIORITY_DEFAULT,
+ utils::GlibRunClosure,
+ callback,
+ utils::GlibDestroyClosure);
return true;
}
CHECK_EQ(reinterpret_cast<Closure*>(NULL), callback_);
@@ -53,6 +57,7 @@
callback_ = NULL;
// This may indirectly call back into ResolveProxiesForUrl():
callback->Run();
+ delete callback;
}
} // namespace chromeos_update_engine
diff --git a/http_fetcher_unittest.cc b/http_fetcher_unittest.cc
index 74c6a16..d7713c5 100644
--- a/http_fetcher_unittest.cc
+++ b/http_fetcher_unittest.cc
@@ -35,10 +35,10 @@
using std::vector;
using base::TimeDelta;
-using testing::_;
-using testing::SetArgumentPointee;
using testing::DoAll;
using testing::Return;
+using testing::SetArgumentPointee;
+using testing::_;
namespace {
@@ -198,6 +198,7 @@
: mock_connection_manager_(&fake_system_state_) {
fake_system_state_.set_connection_manager(&mock_connection_manager_);
}
+ virtual ~AnyHttpFetcherTest() {}
virtual HttpFetcher* NewLargeFetcher(size_t num_proxies) = 0;
HttpFetcher* NewLargeFetcher() {
@@ -611,8 +612,9 @@
EXPECT_FALSE(once_);
EXPECT_TRUE(callback_once_);
callback_once_ = false;
- // |fetcher| can be destroyed during this callback.
- fetcher_.reset(NULL);
+ // The fetcher could have a callback scheduled on the ProxyResolver that
+ // can fire after this callback. We wait until the end of the test to
+ // delete the fetcher.
}
void TerminateTransfer() {
CHECK(once_);
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index 2c3fd4d..be5df41 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -16,9 +16,9 @@
#include "update_engine/real_dbus_wrapper.h"
#include "update_engine/utils.h"
-using google::protobuf::NewCallback;
-using std::max;
+using google::protobuf::NewPermanentCallback;
using std::make_pair;
+using std::max;
using std::string;
// This is a concrete implementation of HttpFetcher that uses libcurl to do the
@@ -251,7 +251,7 @@
url_ = url;
if (!ResolveProxiesForUrl(
url_,
- NewCallback(this, &LibcurlHttpFetcher::ProxiesResolved))) {
+ NewPermanentCallback(this, &LibcurlHttpFetcher::ProxiesResolved))) {
LOG(ERROR) << "Couldn't resolve proxies";
if (delegate_)
delegate_->TransferComplete(this, false);
diff --git a/proxy_resolver.cc b/proxy_resolver.cc
index 43754be..f9905bb 100644
--- a/proxy_resolver.cc
+++ b/proxy_resolver.cc
@@ -21,12 +21,16 @@
bool DirectProxyResolver::GetProxiesForUrl(const std::string& url,
ProxiesResolvedFn callback,
void* data) {
- google::protobuf::Closure* closure =
- google::protobuf::NewCallback(this,
- &DirectProxyResolver::ReturnCallback,
- callback,
- data);
- idle_callback_id_ = g_idle_add(utils::GlibRunClosure, closure);
+ google::protobuf::Closure* closure = google::protobuf::NewPermanentCallback(
+ this,
+ &DirectProxyResolver::ReturnCallback,
+ callback,
+ data);
+ idle_callback_id_ = g_idle_add_full(
+ G_PRIORITY_DEFAULT,
+ utils::GlibRunClosure,
+ closure,
+ utils::GlibDestroyClosure);
return true;
}
diff --git a/utils.cc b/utils.cc
index b32a369..2664f9c 100644
--- a/utils.cc
+++ b/utils.cc
@@ -5,17 +5,17 @@
#include "update_engine/utils.h"
#include <attr/xattr.h>
-#include <sys/mount.h>
-#include <sys/resource.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <sys/wait.h>
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <sys/mount.h>
+#include <sys/resource.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/wait.h>
#include <unistd.h>
#include <algorithm>
@@ -850,6 +850,10 @@
return FALSE;
}
+void GlibDestroyClosure(gpointer data) {
+ delete reinterpret_cast<google::protobuf::Closure*>(data);
+}
+
string FormatSecs(unsigned secs) {
return FormatTimeDelta(TimeDelta::FromSeconds(secs));
}
diff --git a/utils.h b/utils.h
index e83db5c..d2a0fc2 100644
--- a/utils.h
+++ b/utils.h
@@ -318,9 +318,12 @@
// success, false otherwise.
bool SetCpuShares(CpuShares shares);
-// Assumes data points to a Closure. Runs it and returns FALSE;
+// Assumes |data| points to a Closure. Runs it and returns FALSE;
gboolean GlibRunClosure(gpointer data);
+// Destroys the Closure pointed by |data|.
+void GlibDestroyClosure(gpointer data);
+
// Converts seconds into human readable notation including days, hours, minutes
// and seconds. For example, 185 will yield 3m5s, 4300 will yield 1h11m40s, and
// 360000 will yield 4d4h0m0s. Zero padding not applied. Seconds are always