-
Notifications
You must be signed in to change notification settings - Fork 373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(rest): prevent libcurl callback from reading bad address #14615
fix(rest): prevent libcurl callback from reading bad address #14615
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14615 +/- ##
=======================================
Coverage 93.59% 93.59%
=======================================
Files 2316 2316
Lines 207107 207113 +6
=======================================
+ Hits 193839 193846 +7
+ Misses 13268 13267 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you were able to reproduce the crash, can it be made into a regression test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I was unable to reproduce it.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
// error, typically a timeout, has occurred. Instruct curl to not attempt to | ||
// send anymore data on this handle. If curl_closed_ is true, then the handle | ||
// has already been recycled and attempting to set an option on it will error. | ||
if (!curl_closed_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work in the presence of exceptions? Can exceptions happen at this layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion
google/cloud/internal/curl_impl.cc
line 533 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Does this work in the presence of exceptions? Can exceptions happen at this layer?
No it does not work in the presence of exceptions. Unsure if recoverable exceptions could occur at this layer. But, created #14617 to use RAII to ensure that we update CURLOPT_READFUNCTION
regardless of how we leave this function scope.
For a Patch, Post, or Put request,
curl_multi_perform
should result in all the data from the associatedWriteVector
being consumed before it returns. Except in the case of a timeout, in which case we do not want libcurl to attempt to transmit the rest of the data while we read any remaining response data to populate theStatus
error message.fixes #14614
This change is