MultiRangeHttpFetcher::ReceivedBytes should signal the caller correctly.
HttpFetcherDelegate::ReceivedBytes requires a delegate to return false
to signal the caller to terminate. However, the overload in
MultiRangeHttpFetcher::ReceivedBytes missed that and wrongly returned
true instead.
As a result, we observed the following crash while running an ASAN
build.
FileFetcher::OnReadDoneCallback()
|
| called its delegate of MultiRangeHttpFetcher
|
-- MultiRangeHttpFetcher::ReceivedBytes()
|
| requested fetchers to terminate
|
-- fetcher->TerminateTransfer()
|
|-- MultiRangeHttpFetcher::TerminateTransfer()
|
-- FileFetcher::TerminateTransfer()
|
-- MultiRangeHttpFetcher::TransferTerminated()
|
|-- DownloadAction::TransferTerminated()
|
|-- ActionProcessor::ActionComplete()
|
| DownloadAction / MultiRangeHttpFetcher /
| FileFetcher all destroyed
|
| (but it didn't signal the caller to terminate)
|
| FileFetcher proceeded to call FileFetcher::ScheduleRead() that accessed
| already freed memory
While fixing the above issue, a separate bug in
LibcurlHttpFetcher::LibcurlWrite was uncovered, where in_write_callback_
wasn't properly reset.
Bug: 120577143
Test: Build and flash an ASAN build on taimen. Applying a payload with
file:// no longer crashes with tag-mismatch.
Test: Run update_engine_unittests.
Change-Id: I66c862ae40accbfaddc41fb8590b152c2169eea6
diff --git a/common/multi_range_http_fetcher.cc b/common/multi_range_http_fetcher.cc
index 0a19c6a..d39b7f9 100644
--- a/common/multi_range_http_fetcher.cc
+++ b/common/multi_range_http_fetcher.cc
@@ -111,6 +111,7 @@
pending_transfer_ended_ = true;
LOG(INFO) << "Terminating transfer.";
fetcher->TerminateTransfer();
+ return false;
}
return true;
}
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index 50ddeb0..7cf3341 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -544,10 +544,18 @@
}
}
bytes_downloaded_ += payload_size;
- in_write_callback_ = true;
- if (delegate_ && !delegate_->ReceivedBytes(this, ptr, payload_size))
- return payload_size;
- in_write_callback_ = false;
+ if (delegate_) {
+ in_write_callback_ = true;
+ auto should_terminate = !delegate_->ReceivedBytes(this, ptr, payload_size);
+ in_write_callback_ = false;
+ if (should_terminate) {
+ LOG(INFO) << "Requesting libcurl to terminate transfer.";
+ // Returning an amount that differs from the received size signals an
+ // error condition to libcurl, which will cause the transfer to be
+ // aborted.
+ return 0;
+ }
+ }
return payload_size;
}