From 9a1941fab908dc0e80ace4b8650001920abd891b Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Mon, 11 Jul 2022 14:08:38 -0700 Subject: [PATCH] crashdump_uploader: fix memory leaks & use-after-frees These `GoogleCrashdumpUploader` instances need to be cleaned up; place them on the stack. Doing this unmasks another bug in this code: the `MockLibcurlWrapper` instance we're passing into these `GoogleCrashdumpUploader`s becomes owned by the `GoogleCrashdumpUploader` in question. Putting them on the stack makes `free()` unhappy when the `GoogleCrashdumpUploader` they're given to gets destructed. Bug: b:235999011 Change-Id: I5d0424a1c09d32ea34a8fa6f5e52d3695ee6e857 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3756172 Reviewed-by: Mike Frysinger --- src/common/linux/google_crashdump_uploader.cc | 36 +++++----- src/common/linux/google_crashdump_uploader.h | 10 +-- .../linux/google_crashdump_uploader_test.cc | 72 ++++++------------- 3 files changed, 47 insertions(+), 71 deletions(-) diff --git a/src/common/linux/google_crashdump_uploader.cc b/src/common/linux/google_crashdump_uploader.cc index a0d940b6..814c4cae 100644 --- a/src/common/linux/google_crashdump_uploader.cc +++ b/src/common/linux/google_crashdump_uploader.cc @@ -35,6 +35,7 @@ #include #include +#include #include "common/using_std_string.h" @@ -51,7 +52,7 @@ GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product, const string& crash_server, const string& proxy_host, const string& proxy_userpassword) { - LibcurlWrapper* http_layer = new LibcurlWrapper(); + std::unique_ptr http_layer{new LibcurlWrapper()}; Init(product, version, guid, @@ -63,21 +64,22 @@ GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product, crash_server, proxy_host, proxy_userpassword, - http_layer); + std::move(http_layer)); } -GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product, - const string& version, - const string& guid, - const string& ptime, - const string& ctime, - const string& email, - const string& comments, - const string& minidump_pathname, - const string& crash_server, - const string& proxy_host, - const string& proxy_userpassword, - LibcurlWrapper* http_layer) { +GoogleCrashdumpUploader::GoogleCrashdumpUploader( + const string& product, + const string& version, + const string& guid, + const string& ptime, + const string& ctime, + const string& email, + const string& comments, + const string& minidump_pathname, + const string& crash_server, + const string& proxy_host, + const string& proxy_userpassword, + std::unique_ptr http_layer) { Init(product, version, guid, @@ -89,7 +91,7 @@ GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product, crash_server, proxy_host, proxy_userpassword, - http_layer); + std::move(http_layer)); } void GoogleCrashdumpUploader::Init(const string& product, @@ -103,7 +105,7 @@ void GoogleCrashdumpUploader::Init(const string& product, const string& crash_server, const string& proxy_host, const string& proxy_userpassword, - LibcurlWrapper* http_layer) { + std::unique_ptr http_layer) { product_ = product; version_ = version; guid_ = guid; @@ -111,7 +113,7 @@ void GoogleCrashdumpUploader::Init(const string& product, ctime_ = ctime; email_ = email; comments_ = comments; - http_layer_.reset(http_layer); + http_layer_ = std::move(http_layer); crash_server_ = crash_server; proxy_host_ = proxy_host; diff --git a/src/common/linux/google_crashdump_uploader.h b/src/common/linux/google_crashdump_uploader.h index a2d0575b..9358afd8 100644 --- a/src/common/linux/google_crashdump_uploader.h +++ b/src/common/linux/google_crashdump_uploader.h @@ -31,11 +31,11 @@ #ifndef COMMON_LINUX_GOOGLE_CRASHDUMP_UPLOADER_H_ #define COMMON_LINUX_GOOGLE_CRASHDUMP_UPLOADER_H_ -#include #include +#include +#include #include "common/linux/libcurl_wrapper.h" -#include "common/scoped_ptr.h" #include "common/using_std_string.h" namespace google_breakpad { @@ -65,7 +65,7 @@ class GoogleCrashdumpUploader { const string& crash_server, const string& proxy_host, const string& proxy_userpassword, - LibcurlWrapper* http_layer); + std::unique_ptr http_layer); void Init(const string& product, const string& version, @@ -78,7 +78,7 @@ class GoogleCrashdumpUploader { const string& crash_server, const string& proxy_host, const string& proxy_userpassword, - LibcurlWrapper* http_layer); + std::unique_ptr http_layer); bool Upload(int* http_status_code, string* http_response_header, string* http_response_body); @@ -86,7 +86,7 @@ class GoogleCrashdumpUploader { private: bool CheckRequiredParametersArePresent(); - scoped_ptr http_layer_; + std::unique_ptr http_layer_; string product_; string version_; string guid_; diff --git a/src/common/linux/google_crashdump_uploader_test.cc b/src/common/linux/google_crashdump_uploader_test.cc index 3d6612e8..e463f2c8 100644 --- a/src/common/linux/google_crashdump_uploader_test.cc +++ b/src/common/linux/google_crashdump_uploader_test.cc @@ -59,21 +59,12 @@ class GoogleCrashdumpUploaderTest : public ::testing::Test { }; TEST_F(GoogleCrashdumpUploaderTest, InitFailsCausesUploadFailure) { - MockLibcurlWrapper m; - EXPECT_CALL(m, Init()).Times(1).WillOnce(Return(false)); - GoogleCrashdumpUploader *uploader = new GoogleCrashdumpUploader("foobar", - "1.0", - "AAA-BBB", - "", - "", - "test@test.com", - "none", - "/tmp/foo.dmp", - "http://foo.com", - "", - "", - &m); - ASSERT_FALSE(uploader->Upload(NULL, NULL, NULL)); + std::unique_ptr m{new MockLibcurlWrapper()}; + EXPECT_CALL(*m, Init()).Times(1).WillOnce(Return(false)); + GoogleCrashdumpUploader uploader("foobar", "1.0", "AAA-BBB", "", "", + "test@test.com", "none", "/tmp/foo.dmp", + "http://foo.com", "", "", std::move(m)); + ASSERT_FALSE(uploader.Upload(NULL, NULL, NULL)); } TEST_F(GoogleCrashdumpUploaderTest, TestSendRequestHappensWithValidParameters) { @@ -83,44 +74,27 @@ TEST_F(GoogleCrashdumpUploaderTest, TestSendRequestHappensWithValidParameters) { ASSERT_NE(fd, -1); close(fd); - MockLibcurlWrapper m; - EXPECT_CALL(m, Init()).Times(1).WillOnce(Return(true)); - EXPECT_CALL(m, AddFile(tempfn, _)).WillOnce(Return(true)); - EXPECT_CALL(m, - SendRequest("http://foo.com",_,_,_,_)).Times(1).WillOnce(Return(true)); - GoogleCrashdumpUploader *uploader = new GoogleCrashdumpUploader("foobar", - "1.0", - "AAA-BBB", - "", - "", - "test@test.com", - "none", - tempfn, - "http://foo.com", - "", - "", - &m); - ASSERT_TRUE(uploader->Upload(NULL, NULL, NULL)); + std::unique_ptr m{new MockLibcurlWrapper()}; + EXPECT_CALL(*m, Init()).Times(1).WillOnce(Return(true)); + EXPECT_CALL(*m, AddFile(tempfn, _)).WillOnce(Return(true)); + EXPECT_CALL(*m, SendRequest("http://foo.com", _, _, _, _)) + .Times(1) + .WillOnce(Return(true)); + GoogleCrashdumpUploader uploader("foobar", "1.0", "AAA-BBB", "", "", + "test@test.com", "none", tempfn, + "http://foo.com", "", "", std::move(m)); + ASSERT_TRUE(uploader.Upload(NULL, NULL, NULL)); } TEST_F(GoogleCrashdumpUploaderTest, InvalidPathname) { - MockLibcurlWrapper m; - EXPECT_CALL(m, Init()).Times(1).WillOnce(Return(true)); - EXPECT_CALL(m, SendRequest(_,_,_,_,_)).Times(0); - GoogleCrashdumpUploader *uploader = new GoogleCrashdumpUploader("foobar", - "1.0", - "AAA-BBB", - "", - "", - "test@test.com", - "none", - "/tmp/foo.dmp", - "http://foo.com", - "", - "", - &m); - ASSERT_FALSE(uploader->Upload(NULL, NULL, NULL)); + std::unique_ptr m{new MockLibcurlWrapper()}; + EXPECT_CALL(*m, Init()).Times(1).WillOnce(Return(true)); + EXPECT_CALL(*m, SendRequest(_,_,_,_,_)).Times(0); + GoogleCrashdumpUploader uploader("foobar", "1.0", "AAA-BBB", "", "", + "test@test.com", "none", "/tmp/foo.dmp", + "http://foo.com", "", "", std::move(m)); + ASSERT_FALSE(uploader.Upload(NULL, NULL, NULL)); } TEST_F(GoogleCrashdumpUploaderTest, TestRequiredParametersMustBePresent) {