Repository navigation
blob/azureblob: fix panic in writer.Close when the upload fails - #3803
Conversation
Signed-off-by: Mateo Lelong <mateo.lelong@datadoghq.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
therayquaza
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3803 +/- ##
==========================================
+ Coverage 80.02% 80.52% +0.50%
==========================================
Files 104 104
Lines 12318 12317 -1
==========================================
+ Hits 9857 9918 +61
+ Misses 2460 2398 -62
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if w.err != nil { | ||
| if closePipeOnError { | ||
| w.pr.CloseWithError(w.err) | ||
| w.pr = nil |
| func (w *writer) Close() (err error) { | ||
| if !w.upload { | ||
| if w.pr != nil { | ||
| if pr := w.pr; pr != nil { |
There was a problem hiding this comment.
And then change the closure to not use a capture variable but rather check w.pr for nilness before trying to close?
There was a problem hiding this comment.
f69808b unsure why we are unsetting the w.pr when failing but I reverted and added another condition in the closure to avoid panics
Signed-off-by: Mateo Lelong <mateo.lelong@datadoghq.com>
Summary
blob/azureblob'swriter.Close()panics with a nil-pointer dereference when the upload fails after the body has been accepted. The error should be returned, not crash the process.Cause: since 0633e97 (#3741),
Closedefers a closure that readsw.pr, but the upload goroutine clearsw.pr(w.pr = nil) whenUploadStreamfails. By the time the deferredw.pr.Close()runs, the field is nil -> panic.Fix
Test
TestWriterCloseCommitErroruses a local server that drains the body, then returns 409BlobAlreadyExists, so the error surfaces fromClose. It panics without this fix and passes with it (verified deterministic,-raceand lint clean).