[vuln] cmd/govulncheck: fix exit code propagation for wrapped scan errors
Ethan Lee (Gerrit)
Ethan Lee has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/govulncheck: fix exit code propagation for wrapped scan errors
Change diff
diff --git a/cmd/govulncheck/main.go b/cmd/govulncheck/main.go
index 73e3370..5bb6132 100644
--- a/cmd/govulncheck/main.go
+++ b/cmd/govulncheck/main.go
@@ -6,6 +6,7 @@
import (
"context"
+ "errors"
"fmt"
"os"
@@ -23,11 +24,14 @@
if err == nil {
err = cmd.Wait()
}
- switch err := err.(type) {
- case nil:
- case interface{ ExitCode() int }:
- os.Exit(err.ExitCode())
- default:
+ if err != nil {
+ var e interface{ ExitCode() int }
+ if errors.As(err, &e) {
+ if _, ok := err.(interface{ ExitCode() int }); !ok {
+ fmt.Fprintln(os.Stderr, err)
+ }
+ os.Exit(e.ExitCode())
+ }
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
diff --git a/cmd/govulncheck/main_test.go b/cmd/govulncheck/main_test.go
index 5613de5..1a19778 100644
--- a/cmd/govulncheck/main_test.go
+++ b/cmd/govulncheck/main_test.go
@@ -7,6 +7,7 @@
import (
"bytes"
"context"
+ "errors"
"flag"
"fmt"
"os"
@@ -155,16 +156,21 @@
return nil, err
}
err := cmd.Wait()
- switch e := err.(type) {
- case nil:
- case interface{ ExitCode() int }:
- err = &cmdtest.ExitCodeErr{Msg: err.Error(), Code: e.ExitCode()}
- if e.ExitCode() == 0 {
- err = nil
+ if err != nil {
+ var e interface{ ExitCode() int }
+ if errors.As(err, &e) {
+ code := e.ExitCode()
+ if _, ok := err.(interface{ ExitCode() int }); !ok {
+ fmt.Fprintln(buf, err)
+ }
+ err = &cmdtest.ExitCodeErr{Msg: err.Error(), Code: code}
+ if code == 0 {
+ err = nil
+ }
+ } else {
+ fmt.Fprintln(buf, err)
+ err = &cmdtest.ExitCodeErr{Msg: err.Error(), Code: 1}
}
- default:
- fmt.Fprintln(buf, err)
- err = &cmdtest.ExitCodeErr{Msg: err.Error(), Code: 1}
}
sorted := buf
if err == nil && isJSONMode(args) {
diff --git a/cmd/govulncheck/testdata/common/testfiles/usage/source_no_packages.ct b/cmd/govulncheck/testdata/common/testfiles/usage/source_no_packages.ct
index 27d44a9..e783d31 100644
--- a/cmd/govulncheck/testdata/common/testfiles/usage/source_no_packages.ct
+++ b/cmd/govulncheck/testdata/common/testfiles/usage/source_no_packages.ct
@@ -1,5 +1,4 @@
#####
# Test message when there are no packages matching the provided pattern (#59623).
-$ govulncheck -show verbose -C ${moddir}/vuln pkg/no-govulncheck/...
-No packages matched the provided pattern.
-No vulnerabilities found.
+$ govulncheck -show verbose -C ${moddir}/vuln pkg/no-govulncheck/... --> FAIL 2
+govulncheck: no packages matched the provided patterns
diff --git a/cmd/govulncheck/testdata/common/testfiles/usage/usage.ct b/cmd/govulncheck/testdata/common/testfiles/usage/usage.ct
index ff2c2ae..345e9b3 100644
--- a/cmd/govulncheck/testdata/common/testfiles/usage/usage.ct
+++ b/cmd/govulncheck/testdata/common/testfiles/usage/usage.ct
@@ -35,15 +35,19 @@
#####
# Not scanning anything.
-$ govulncheck
-No vulnerabilities found.
+$ govulncheck --> FAIL 2
+govulncheck: no package patterns provided
+
+To scan the current module, run: govulncheck ./...
#####
# Reporting version without scanning anything.
-$ govulncheck -version
+$ govulncheck -version --> FAIL 2
Go: go1.18
Scanner: govul...@v1.0.0
DB: testdata/vulndb-v1
DB updated: 2023-04-03 15:57:51 +0000 UTC
-No vulnerabilities found.
+govulncheck: no package patterns provided
+
+To scan the current module, run: govulncheck ./...
Change information
- M cmd/govulncheck/main.go
- M cmd/govulncheck/main_test.go
- M cmd/govulncheck/testdata/common/testfiles/usage/source_no_packages.ct
- M cmd/govulncheck/testdata/common/testfiles/usage/usage.ct
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted![]( "Open in Gerrit")
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Dmitri Shuralyov
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov added 4 comments![]( "Open in Gerrit")
Thanks for working on a fix. I may not be the best reviewer for this since I'm not familiar with x/vuln and conventions or constraints involved here. I left some questions, but if there's someone better to review this, please feel free to add them.
Change-Id: Id94878d231272a9a37817ece747692a1ad539cc6Since the original issue happens only when -short flag is left out, the signal from current trybots isn't telling us whether that issue is fixed. I suggest adding a line here that will ensure at least one longtest builder needs to pass:
```suggestion
Change-Id: Id94878d231272a9a37817ece747692a1ad539cc6
Cq-Include-Trybots: luci.golang.try:x_vuln-gotip-linux-amd64-longtest
```
(The Cq-Include-Trybots footer syntax is documented at https://go.dev/wiki/SlowBots#using-slowbots, under the 'Blocking' section.)
Or if there's more to do to beyond this CL get the longtest builders passing, you can leave this out and use 'For' instead of 'Fixes'.
os.Exit(1)A few questions about this error handling logic. It seems a bit complicated, and I'm not sure if that complexity is required or can be avoided.
I haven't encountered errors.As used with an interface type as the target. (Compare with the examples at https://pkg.go.dev/errors#As that show struct types.) What's the reason both errors.As and a type assertion are both used? Could this be simplified to an unconditional print to stderr and single type assertion instead, something like:
```
fmt.Fprintln(os.Stderr, err)
if e, ok := err.(interface{ ExitCode() int}); ok {
os.Exit(e.ExitCode())
} else {
os.Exit(1)
}
```
Or is the reason errors.As needs to be used because `err` itself may be wrapped in a way that hides the ExitCode method at the top level, such that the type assertion to the interface fails, yet errors.As unwraps the error and finds that something in its tree matches?
(Part of the reason I can't tell what is intended here is that the scan.Cmd.Wait API doesn't talk about how it reports errors and exit codes; see another inline comment about that.)
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee added 3 comments![]( "Open in Gerrit")
Since the original issue happens only when -short flag is left out, the signal from current trybots isn't telling us whether that issue is fixed. I suggest adding a line here that will ensure at least one longtest builder needs to pass:
```suggestion
Change-Id: Id94878d231272a9a37817ece747692a1ad539cc6
Cq-Include-Trybots: luci.golang.try:x_vuln-gotip-linux-amd64-longtest
```(The Cq-Include-Trybots footer syntax is documented at https://go.dev/wiki/SlowBots#using-slowbots, under the 'Blocking' section.)
Or if there's more to do to beyond this CL get the longtest builders passing, you can leave this out and use 'For' instead of 'Fixes'.
Interesting! Thanks for letting me know about this format. :) Done.
os.Exit(1)A few questions about this error handling logic. It seems a bit complicated, and I'm not sure if that complexity is required or can be avoided.
I haven't encountered errors.As used with an interface type as the target. (Compare with the examples at https://pkg.go.dev/errors#As that show struct types.) What's the reason both errors.As and a type assertion are both used? Could this be simplified to an unconditional print to stderr and single type assertion instead, something like:
```
fmt.Fprintln(os.Stderr, err)
if e, ok := err.(interface{ ExitCode() int}); ok {
os.Exit(e.ExitCode())
} else {
os.Exit(1)
}
```Or is the reason errors.As needs to be used because `err` itself may be wrapped in a way that hides the ExitCode method at the top level, such that the type assertion to the interface fails, yet errors.As unwraps the error and finds that something in its tree matches?
(Part of the reason I can't tell what is intended here is that the scan.Cmd.Wait API doesn't talk about how it reports errors and exit codes; see another inline comment about that.)
Your second reason is correct.
`errors.As(err, &e)` is used to retrieve the exit code from nested errors (e.g., wrapped via `fmt.Errorf("context: %w", ...)`). A simple type assertion `err.(interface{ ExitCode() int })` at the top level would fail on wrapped errors, masking the actual exit code (such as exiting with 1 instead of 3 for vulnerabilities).
We also want to prevent duplicate logging. We can't unconditionally print to stderr because some exit-code errors are silent (like errVulnerabilitiesFound with exit code 3, or errHelp with exit code 0).
I addressed the other comment with appropriate documentation. Let me know if that's clear.
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov voted and added 5 comments![]( "Open in Gerrit")
Votes added by Dmitri Shuralyov
| Code-Review | +2 |
5 comments
Change-Id: Id94878d231272a9a37817ece747692a1ad539cc6Ethan LeeSince the original issue happens only when -short flag is left out, the signal from current trybots isn't telling us whether that issue is fixed. I suggest adding a line here that will ensure at least one longtest builder needs to pass:
```suggestion
Change-Id: Id94878d231272a9a37817ece747692a1ad539cc6
Cq-Include-Trybots: luci.golang.try:x_vuln-gotip-linux-amd64-longtest
```(The Cq-Include-Trybots footer syntax is documented at https://go.dev/wiki/SlowBots#using-slowbots, under the 'Blocking' section.)
Or if there's more to do to beyond this CL get the longtest builders passing, you can leave this out and use 'For' instead of 'Fixes'.
Interesting! Thanks for letting me know about this format. :) Done.
Thanks.
It seems to be catching that TestCommand/common is still failing. If there's more work needed to get everything passing and you prefer to make that a separate CL, then it'd make sense to switch 'For' to 'Fixes'. Up to you.
fmt.Fprintln(os.Stderr, err)The logic of checking whether err wraps an exit code error, and whether that error is at the top of the error stack or in the middle is a bit complex, so a clarifying comment can help make this code more readable. Maybe something like:
```suggestion
// If the exit code error was wrapped with another error on top, print full error details to stderr.
// We avoid doing this if it wasn't wrapped since printing a generic "exit code n" message to stderr since is considered unhelpful.
fmt.Fprintln(os.Stderr, err)
```
Perhaps better yet use a named intermediate variable:
```
printErrorToStderr := true
if _, ok := err.(interface{ ExitCode() int }); ok {
// Avoid printing the error to stderr if the exit code error wasn't
// wrapped with another error providing context.
printErrorToStderr = false
}
if printErrorToStderr {
fmt.Fprintln(os.Stderr, err)
}
```
os.Exit(1)Ethan LeeA few questions about this error handling logic. It seems a bit complicated, and I'm not sure if that complexity is required or can be avoided.
I haven't encountered errors.As used with an interface type as the target. (Compare with the examples at https://pkg.go.dev/errors#As that show struct types.) What's the reason both errors.As and a type assertion are both used? Could this be simplified to an unconditional print to stderr and single type assertion instead, something like:
```
fmt.Fprintln(os.Stderr, err)
if e, ok := err.(interface{ ExitCode() int}); ok {
os.Exit(e.ExitCode())
} else {
os.Exit(1)
}
```Or is the reason errors.As needs to be used because `err` itself may be wrapped in a way that hides the ExitCode method at the top level, such that the type assertion to the interface fails, yet errors.As unwraps the error and finds that something in its tree matches?
(Part of the reason I can't tell what is intended here is that the scan.Cmd.Wait API doesn't talk about how it reports errors and exit codes; see another inline comment about that.)
Your second reason is correct.
`errors.As(err, &e)` is used to retrieve the exit code from nested errors (e.g., wrapped via `fmt.Errorf("context: %w", ...)`). A simple type assertion `err.(interface{ ExitCode() int })` at the top level would fail on wrapped errors, masking the actual exit code (such as exiting with 1 instead of 3 for vulnerabilities).
We also want to prevent duplicate logging. We can't unconditionally print to stderr because some exit-code errors are silent (like errVulnerabilitiesFound with exit code 3, or errHelp with exit code 0).
I addressed the other comment with appropriate documentation. Let me know if that's clear.
Thanks for clarifying.
if _, ok := err.(interface{ ExitCode() int }); !ok {Similar suggestion here as in cmd/govulncheck/main.go.
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Dmitri Shuralyov, +1 by Dmitri Shuralyov
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Dmitri Shuralyov
- Dmitri Shuralyov
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee added 3 comments![]( "Open in Gerrit")
The logic of checking whether err wraps an exit code error, and whether that error is at the top of the error stack or in the middle is a bit complex, so a clarifying comment can help make this code more readable. Maybe something like:
```suggestion
// If the exit code error was wrapped with another error on top, print full error details to stderr.
// We avoid doing this if it wasn't wrapped since printing a generic "exit code n" message to stderr since is considered unhelpful.
fmt.Fprintln(os.Stderr, err)
```Perhaps better yet use a named intermediate variable:
```
printErrorToStderr := true
if _, ok := err.(interface{ ExitCode() int }); ok {
// Avoid printing the error to stderr if the exit code error wasn't
// wrapped with another error providing context.
printErrorToStderr = false
}
if printErrorToStderr {
fmt.Fprintln(os.Stderr, err)
}
```
Done
if _, ok := err.(interface{ ExitCode() int }); !ok {Similar suggestion here as in cmd/govulncheck/main.go.
Done
Related details
- Dmitri Shuralyov
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dmitri Shuralyov
| Code-Review | +2 |
1 comment
Thanks.
- Dmitri Shuralyov
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov voted Code-Review+1![]( "Open in Gerrit")
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee submitted the change![]( "Open in Gerrit")
Change information
cmd/govulncheck: fix exit code propagation for wrapped scan errors
Previously, exit codes were masked improperly. Ensure that the original
error codes are propagated.
Fixes golang/go#78694
- M cmd/govulncheck/main.go
- M cmd/govulncheck/main_test.go
- M cmd/govulncheck/testdata/common/testfiles/usage/format.ct
- M cmd/govulncheck/testdata/common/testfiles/usage/source_no_packages.ct
- M cmd/govulncheck/testdata/common/testfiles/usage/usage.ct
- M internal/scan/run.go
- M scan/scan.go
Code-Review: +2 by Dmitri Shuralyov, +1 by Dmitri Shuralyov
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dustin Turner (Gerrit)
Dustin Turner voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |