fix(image): resolve scan deadlock when error occurs in slow mode

[mpoindexter]

Description

See #4335

Related issues

• Close Trivy can hang if an error occurs scanning a container image with --slow option #4343

Checklist

• I've read the guidelines for contributing to this repository.
• I've followed the conventions in the PR title.
• I've added tests that prove my fix is effective or that my feature works.

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[DmitriyLewen]

Hello @mpoindexter
Thanks for your work!

As you said in #4335 i reproduced this case, but i can't find information why semaphore blocks channel. Do you have docs about this case?
Also i found that there is no problem if buffered channel is used (i mean errCh := make(chan error, 1))

[mpoindexter]

It's not that the semaphore blocks a channel send directly, the channel send is blocked because it's an unbuffered channel, and nothing is yet reading from the channel. In turn, the goroutine that will eventually be responsible for reading from the channel is blocked because the goroutine trying to send on the channel holds the semaphore which allows at most one holder in slow mode. Using a buffered channel sort of fixes the problem, but for it to be a full fix the buffer size of the channel must be equivalent to max number of errors that could happen, not 1. Otherwise if there are errors on more than one layers the problem can occur.

[DmitriyLewen]

Thanks that you explained in more detail and fast answer!

[DmitriyLewen]

This way looks good for me.
Thanks for your work @mpoindexter .
@knqyf263 I approved this PR.

[knqyf263]

What if putting limit.Acquire into goroutine? The downside is the number of layers would create a goroutine. But I think it is acceptable since goroutine is lightweight, and the number of layers wouldn't be in the thousands.

[mpoindexter]

@knqyf263 I moved the limit.Acquire into goroutine. I think that the select blocks around all the sends from the goroutine are needed since the scan is aborted on first error and hence we need to be able to avoid sending for other goroutines once a single one has errored

[knqyf263]

We may want to use errgroup in this case.
https://pkg.go.dev/golang.org/x/sync/errgroup

[mpoindexter]

OK, updated to use errgroup

[knqyf263]

I think it is acceptable to run goroutine even after an error. What if removing this error check?

[mpoindexter]

I think it would be worse to remove it - there's no correctness problem with removing the check, but inspecting a layer can be quite expensive, so it seems like we should stop doing it if we already know that we're going to get an error result when we call group.Wait()

[knqyf263]

groupCtx is not updated in the loop. Is there any specific reason to overwrite ctx every time here?

[mpoindexter]

I thought it looked cleaner to pass ctx in the body of the goroutine. The code previously took ctx as an argument to the goroutine, but with errgroup we can't pass arguments to the goroutine, so just binding some variables was the replacement. We could just change to use groupCtx within the goroutine, let me know.