Fix use-after-free in HttpFetcher's code.
Calling HttpFetcherDelegate::TransferComplete() or TransferTerminated()
on the transfer delegate is allowed to destroy the HttpFetcher that
called it.
While we don't do this from our code, the unittests excercise this path
destroying the HttpFetcher. This code prevents a use-after-free once
the TransferComplete() delegate method is called from CurlPerformOnce().
Bug: chromium:595068
TEST=USE="clang asan" FEATURES=test emerge-link update_engine
Change-Id: I73317eae092ed4ac1ccad5c3fc68b25ad8bb387c
diff --git a/common/libcurl_http_fetcher.cc b/common/libcurl_http_fetcher.cc
index b2dffc2..5b61263 100644
--- a/common/libcurl_http_fetcher.cc
+++ b/common/libcurl_http_fetcher.cc
@@ -424,6 +424,7 @@
LOG(INFO) << "No further proxies, indicating transfer complete";
if (delegate_)
delegate_->TransferComplete(this, false); // signal fail
+ return;
}
} else if ((transfer_size_ >= 0) && (bytes_downloaded_ < transfer_size_)) {
if (!ignore_failure_)
@@ -437,15 +438,15 @@
LOG(INFO) << "Reached max attempts (" << retry_count_ << ")";
if (delegate_)
delegate_->TransferComplete(this, false); // signal fail
- } else {
- // Need to restart transfer
- LOG(INFO) << "Restarting transfer to download the remaining bytes";
- MessageLoop::current()->PostDelayedTask(
- FROM_HERE,
- base::Bind(&LibcurlHttpFetcher::RetryTimeoutCallback,
- base::Unretained(this)),
- TimeDelta::FromSeconds(retry_seconds_));
+ return;
}
+ // Need to restart transfer
+ LOG(INFO) << "Restarting transfer to download the remaining bytes";
+ MessageLoop::current()->PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&LibcurlHttpFetcher::RetryTimeoutCallback,
+ base::Unretained(this)),
+ TimeDelta::FromSeconds(retry_seconds_));
} else {
LOG(INFO) << "Transfer completed (" << http_response_code_
<< "), " << bytes_downloaded_ << " bytes downloaded";
@@ -453,7 +454,11 @@
bool success = IsHttpResponseSuccess();
delegate_->TransferComplete(this, success);
}
+ return;
}
+ // If we reach this point is because TransferComplete() was not called in any
+ // of the previous branches. The delegate is allowed to destroy the object
+ // once TransferComplete is called so this would be illegal.
ignore_failure_ = false;
}