Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fifo.Close(): prevent possible panic if fifo is nil #32

Merged
merged 1 commit into from
Sep 15, 2021
Merged

fifo.Close(): prevent possible panic if fifo is nil #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions fifo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ func (f *fifo) Write(b []byte) (int, error) {
// before open(2) has returned and fifo was never opened.
func (f *fifo) Close() (retErr error) {
for {
if f == nil {
return
Copy link
Member Author

@thaJeztah thaJeztah Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This method can also be used before open(2) has returned and fifo was never opened.

Also not sure if we should

  • return an error in this case (currently it would use retErr, if set)
  • return nil (which would unset retErr if it was set before)

Copy link
Member

Choose a reason for hiding this comment

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

I think the caller should check for nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could it become nil while we're in this for{} loop?

Copy link
Member

Choose a reason for hiding this comment

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

I should hope not... if so then that would be the bug that should be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... so I suspect that something like that actually happens (somehow); following that trace, the panic followed from this code; https://github.com/containerd/containerd/blob/269548fa27e0089a8b8278fc4fc781d7f65a939b/cio/io.go#L199-L206

func (c *cio) Close() error {
	var lastErr error
	for _, closer := range c.closers {
		if closer == nil {
			continue
		}
		if err := closer.Close(); err != nil {
			lastErr = err
		}
	}
	return lastErr
}

So there's already a nil check there. So only other explanation would be there there's some concurrency happening, in which case do we need locking?

Copy link
Member

Choose a reason for hiding this comment

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

could have been explain by the infamous interface is nil problem, either we make sure that we dont pass nil io.Closer to cio, or do a type check ( yuck), or we check inside fifo.Close for nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry, missed your comment here, @dqminh - do you mean we should use a different fix, or is the current "fix" ok?

Copy link
Member

Choose a reason for hiding this comment

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

i think the best case is that we need to make sure the caller doesn't pass a nil interface as that can be nasty too if we call anything other than Close ( which is fixed in this way ). The current check we are doing here is not the end of the world, but ideally i don't think we need it at this place.

}

select {
case <-f.closed:
f.handle.Close()
Expand Down