Skip to content

Fix panic when bufio Reader called in 2 goroutines#2687

Merged
estesp merged 1 commit intocontainerd:masterfrom
dmcgowan:fix-pigz-panic
Sep 27, 2018
Merged

Fix panic when bufio Reader called in 2 goroutines#2687
estesp merged 1 commit intocontainerd:masterfrom
dmcgowan:fix-pigz-panic

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

A panic was seen related to the buffer being reset in one goroutine while being read in another. In the case of pigz an early cancellation will cause the reader to close, resetting the buffer and signaling the process to shut down, but races since the process must stop reading before the reset otherwise the a panic may occur. This fix guarantees that the bufio is always reset and returned to the pool on the same goroutine that is doing the read. If a buffer is not fully read the buffered reader should just be discarded and not returned back to the pool.

A panic was seen related to the buffer being reset in
one goroutine while being read in another. In the case
of pigz an early cancellation will cause the reader to
close, resetting the buffer and signaling the process
to shut down, but races since the process must stop
reading before the reset otherwise the a panic may occur.
This fix guarantees that the bufio is always reset and
returned to the pool on the same goroutine that is
doing the read. If a buffer is not fully read the
buffered reader should just be discarded and not
returned back to the pool.

Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan dmcgowan added this to the 1.2 milestone Sep 27, 2018
@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 27, 2018

Codecov Report

Merging #2687 into master will increase coverage by 0.08%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2687      +/-   ##
==========================================
+ Coverage      45%   45.09%   +0.08%     
==========================================
  Files          92       92              
  Lines       10114    10132      +18     
==========================================
+ Hits         4552     4569      +17     
  Misses       4842     4842              
- Partials      720      721       +1
Flag Coverage Δ
#linux 48.84% <72.41%> (+0.1%) ⬆️
#windows 41.87% <68.75%> (+0.1%) ⬆️
Impacted Files Coverage Δ
archive/compression/compression.go 58.69% <68.75%> (+5.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f88d3e5...db358a9. Read the comment docs.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Sep 27, 2018

LGTM 👍

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp merged commit df60d32 into containerd:master Sep 27, 2018
@dmcgowan dmcgowan deleted the fix-pigz-panic branch September 10, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants