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.

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

libcontainer: logs: safely close channel and ensure error propagation in ForwardLogs in logs.go #4656

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AntonMoryakov
Copy link

Static analyzer reported:
Use of value 'done' at logs.go:38 after release at logs.go:35 by calling function 'logs.ForwardLogs$1' at logs.go:25.

Corrections explained:

  • Added defer close(done) to ensure the channel is closed after the goroutine finishes.
  • Removed explicit close(done) after sending the error to avoid potential panics.
  • Ensured that errors from s.Scan() are properly propagated to the done channel.

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov [email protected]

… in ForwardLogs in logs.go

Static analyzer reported:
Use of value 'done' at logs.go:38 after release at logs.go:35 by calling function 'logs.ForwardLogs$1' at logs.go:25.

Corrections explained:
- Added `defer close(done)` to ensure the channel is closed after the goroutine finishes.
- Removed explicit `close(done)` after sending the error to avoid potential panics.
- Ensured that errors from `s.Scan()` are properly propagated to the `done` channel.

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov <[email protected]>

Signed-off-by: AntonMoryakov <[email protected]>
@cyphar
Copy link
Member

cyphar commented Mar 3, 2025

The static analyser is wrong here, the code is being run in a goroutine so it's not "used after" and this pattern is an incredibly common Go pattern. Given that the analyser appears to not understand goroutines (or inline functions?), I wonder if the defer close(...) line is silencing it because the analyser thinks that go func() { ... }() is a regular scope (it isn't).

AFAICS there is no need to do a defer here (yes, defer is no longer a performance issue in Go but I don't think it makes the code easier to understand).

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.

2 participants