yuzu-emu/breakpad
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

crashdump_uploader: fix memory leaks & use-after-frees

Browse source 
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 <vapier@chromium.org>
This commit is contained in:
George Burgess IV 2022-07-11 14:08:38 -07:00 committed by George Burgess
parent 4d7cd09800
commit 9a1941fab9
3 changed files with 47 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

36
src/common/linux/google_crashdump_uploader.cc
View file

@ -35,6 +35,7 @@
#include <unistd.h> #include <unistd.h>
#include <iostream> #include <iostream>
#include <utility>
#include "common/using_std_string.h" #include "common/using_std_string.h"
@ -51,7 +52,7 @@ GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product,
const string& crash_server, const string& crash_server,
const string& proxy_host, const string& proxy_host,
const string& proxy_userpassword) { const string& proxy_userpassword) {
LibcurlWrapper* http_layer = new LibcurlWrapper(); std::unique_ptr<LibcurlWrapper> http_layer{new LibcurlWrapper()};
Init(product, Init(product,
version, version,
guid, guid,
@ -63,21 +64,22 @@ GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product,
crash_server, crash_server,
proxy_host, proxy_host,
proxy_userpassword, proxy_userpassword,
http_layer); std::move(http_layer));
} }
GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product, GoogleCrashdumpUploader::GoogleCrashdumpUploader(
const string& version, const string& product,
const string& guid, const string& version,
const string& ptime, const string& guid,
const string& ctime, const string& ptime,
const string& email, const string& ctime,
const string& comments, const string& email,
const string& minidump_pathname, const string& comments,
const string& crash_server, const string& minidump_pathname,
const string& proxy_host, const string& crash_server,
const string& proxy_userpassword, const string& proxy_host,
LibcurlWrapper* http_layer) { const string& proxy_userpassword,
std::unique_ptr<LibcurlWrapper> http_layer) {
Init(product, Init(product,
version, version,
guid, guid,
@ -89,7 +91,7 @@ GoogleCrashdumpUploader::GoogleCrashdumpUploader(const string& product,
crash_server, crash_server,
proxy_host, proxy_host,
proxy_userpassword, proxy_userpassword,
http_layer); std::move(http_layer));
} }
void GoogleCrashdumpUploader::Init(const string& product, void GoogleCrashdumpUploader::Init(const string& product,
@ -103,7 +105,7 @@ void GoogleCrashdumpUploader::Init(const string& product,
const string& crash_server, const string& crash_server,
const string& proxy_host, const string& proxy_host,
const string& proxy_userpassword, const string& proxy_userpassword,
LibcurlWrapper* http_layer) { std::unique_ptr<LibcurlWrapper> http_layer) {
product_ = product; product_ = product;
version_ = version; version_ = version;
guid_ = guid; guid_ = guid;
@ -111,7 +113,7 @@ void GoogleCrashdumpUploader::Init(const string& product,
ctime_ = ctime; ctime_ = ctime;
email_ = email; email_ = email;
comments_ = comments; comments_ = comments;
http_layer_.reset(http_layer); http_layer_ = std::move(http_layer);
crash_server_ = crash_server; crash_server_ = crash_server;
proxy_host_ = proxy_host; proxy_host_ = proxy_host;

10
src/common/linux/google_crashdump_uploader.h
View file

@ -31,11 +31,11 @@
#ifndef COMMON_LINUX_GOOGLE_CRASHDUMP_UPLOADER_H_ #ifndef COMMON_LINUX_GOOGLE_CRASHDUMP_UPLOADER_H_
#define COMMON_LINUX_GOOGLE_CRASHDUMP_UPLOADER_H_ #define COMMON_LINUX_GOOGLE_CRASHDUMP_UPLOADER_H_
#include <string>
#include <map> #include <map>
#include <memory>
#include <string>
#include "common/linux/libcurl_wrapper.h" #include "common/linux/libcurl_wrapper.h"
#include "common/scoped_ptr.h"
#include "common/using_std_string.h" #include "common/using_std_string.h"
namespace google_breakpad { namespace google_breakpad {
@ -65,7 +65,7 @@ class GoogleCrashdumpUploader {
const string& crash_server, const string& crash_server,
const string& proxy_host, const string& proxy_host,
const string& proxy_userpassword, const string& proxy_userpassword,
LibcurlWrapper* http_layer); std::unique_ptr<LibcurlWrapper> http_layer);
void Init(const string& product, void Init(const string& product,
const string& version, const string& version,
@ -78,7 +78,7 @@ class GoogleCrashdumpUploader {
const string& crash_server, const string& crash_server,
const string& proxy_host, const string& proxy_host,
const string& proxy_userpassword, const string& proxy_userpassword,
LibcurlWrapper* http_layer); std::unique_ptr<LibcurlWrapper> http_layer);
bool Upload(int* http_status_code, bool Upload(int* http_status_code,
string* http_response_header, string* http_response_header,
string* http_response_body); string* http_response_body);
@ -86,7 +86,7 @@ class GoogleCrashdumpUploader {
private: private:
bool CheckRequiredParametersArePresent(); bool CheckRequiredParametersArePresent();
scoped_ptr<LibcurlWrapper> http_layer_; std::unique_ptr<LibcurlWrapper> http_layer_;
string product_; string product_;
string version_; string version_;
string guid_; string guid_;

72
src/common/linux/google_crashdump_uploader_test.cc
View file

@ -59,21 +59,12 @@ class GoogleCrashdumpUploaderTest : public ::testing::Test {
}; };
TEST_F(GoogleCrashdumpUploaderTest, InitFailsCausesUploadFailure) { TEST_F(GoogleCrashdumpUploaderTest, InitFailsCausesUploadFailure) {
MockLibcurlWrapper m; std::unique_ptr<MockLibcurlWrapper> m{new MockLibcurlWrapper()};
EXPECT_CALL(m, Init()).Times(1).WillOnce(Return(false)); EXPECT_CALL(*m, Init()).Times(1).WillOnce(Return(false));
GoogleCrashdumpUploader *uploader = new GoogleCrashdumpUploader("foobar", GoogleCrashdumpUploader uploader("foobar", "1.0", "AAA-BBB", "", "",
"1.0", "test@test.com", "none", "/tmp/foo.dmp",
"AAA-BBB", "http://foo.com", "", "", std::move(m));
"", ASSERT_FALSE(uploader.Upload(NULL, NULL, NULL));
"",
"test@test.com",
"none",
"/tmp/foo.dmp",
"http://foo.com",
"",
"",
&m);
ASSERT_FALSE(uploader->Upload(NULL, NULL, NULL));
} }
TEST_F(GoogleCrashdumpUploaderTest, TestSendRequestHappensWithValidParameters) { TEST_F(GoogleCrashdumpUploaderTest, TestSendRequestHappensWithValidParameters) {
@ -83,44 +74,27 @@ TEST_F(GoogleCrashdumpUploaderTest, TestSendRequestHappensWithValidParameters) {
ASSERT_NE(fd, -1); ASSERT_NE(fd, -1);
close(fd); close(fd);
MockLibcurlWrapper m; std::unique_ptr<MockLibcurlWrapper> m{new MockLibcurlWrapper()};
EXPECT_CALL(m, Init()).Times(1).WillOnce(Return(true)); EXPECT_CALL(*m, Init()).Times(1).WillOnce(Return(true));
EXPECT_CALL(m, AddFile(tempfn, _)).WillOnce(Return(true)); EXPECT_CALL(*m, AddFile(tempfn, _)).WillOnce(Return(true));
EXPECT_CALL(m, EXPECT_CALL(*m, SendRequest("http://foo.com", _, _, _, _))
SendRequest("http://foo.com",_,_,_,_)).Times(1).WillOnce(Return(true)); .Times(1)
GoogleCrashdumpUploader *uploader = new GoogleCrashdumpUploader("foobar", .WillOnce(Return(true));
"1.0", GoogleCrashdumpUploader uploader("foobar", "1.0", "AAA-BBB", "", "",
"AAA-BBB", "test@test.com", "none", tempfn,
"", "http://foo.com", "", "", std::move(m));
"", ASSERT_TRUE(uploader.Upload(NULL, NULL, NULL));
"test@test.com",
"none",
tempfn,
"http://foo.com",
"",
"",
&m);
ASSERT_TRUE(uploader->Upload(NULL, NULL, NULL));
} }
TEST_F(GoogleCrashdumpUploaderTest, InvalidPathname) { TEST_F(GoogleCrashdumpUploaderTest, InvalidPathname) {
MockLibcurlWrapper m; std::unique_ptr<MockLibcurlWrapper> m{new MockLibcurlWrapper()};
EXPECT_CALL(m, Init()).Times(1).WillOnce(Return(true)); EXPECT_CALL(*m, Init()).Times(1).WillOnce(Return(true));
EXPECT_CALL(m, SendRequest(_,_,_,_,_)).Times(0); EXPECT_CALL(*m, SendRequest(_,_,_,_,_)).Times(0);
GoogleCrashdumpUploader *uploader = new GoogleCrashdumpUploader("foobar", GoogleCrashdumpUploader uploader("foobar", "1.0", "AAA-BBB", "", "",
"1.0", "test@test.com", "none", "/tmp/foo.dmp",
"AAA-BBB", "http://foo.com", "", "", std::move(m));
"", ASSERT_FALSE(uploader.Upload(NULL, NULL, NULL));
"",
"test@test.com",
"none",
"/tmp/foo.dmp",
"http://foo.com",
"",
"",
&m);
ASSERT_FALSE(uploader->Upload(NULL, NULL, NULL));
} }
TEST_F(GoogleCrashdumpUploaderTest, TestRequiredParametersMustBePresent) { TEST_F(GoogleCrashdumpUploaderTest, TestRequiredParametersMustBePresent) {