Check errors that were discarded before use - #151
Merged
Conversation
InitUAPI ignored the error from UAPIOpen and passed the resulting nil
*os.File to UAPIListen, which hands it to net.FileListener. That panics
rather than returning an error, because net/file.go reads f.Name() for
the error message before checking the file. Starting a second instance on
an interface ("unix socket in use") or starting without root (MkdirAll on
the socket directory) both reach it, so the two likeliest startup
mistakes print a nil-pointer stack trace instead of the message that was
already written for them. Linux and darwin were identical here.
Two smaller ones of the same shape: the pubkey MarshalText error in the
key command was overwritten by the Fprintln error before it was tested,
so a failure there printed an empty public key and exited 0, and
BundleConfig dropped the yaml.Marshal error and went on to sign the
result.
Owner
|
Thanks for this contribution |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
staticcheck (SA4006) flags four places where an error is assigned and then discarded before anything tests it. One of them turns a handled startup failure into a panic.
InitUAPIpanics instead of reporting why it failedcore/sys_linux.go:14andcore/sys_darwin.go:13are identical:UAPIOpenreturns(nil, err)on four paths inpolyamide/ipc/uapi_unix.go:MkdirAllon the socket directory,ResolveUnixAddr,errors.New("unix socket in use"), andos.Removeof a stale socket. The nil*os.FilereachesUAPIListen, which callsnet.FileListener(file).net.FileListener(nil)does not return an error, it panics.net/file.go:36readsf.Name()to build the error message before anything checks the file:So the two likeliest ways to start nylon wrong both end in a nil-pointer stack trace rather than the message already written for them: starting a second instance on an interface that already has a UAPI socket gives
unix socket in use, and starting without root fails theMkdirAll.InitUAPIhas one caller,core/sys_physical.go:55, on the startup path.Worth noting the darwin one is invisible to a normal
staticcheck ./..., since it analyses a single GOOS and the build-tagged file is not in the package otherwise. It only appears underGOOS=darwin.The two smaller ones
cmd/utils.go:60—pubKeyStr, err := privKey.Pubkey().MarshalText()is immediately followed by_, err = fmt.Fprintln(...), so the marshal error is overwritten before it is tested. The private key branch just above does check it. On a failure,nylon keyprints an empty public key to stderr and exits 0.state/distribution.go:80—plainText, err := yaml.Marshal(cfg)goes straight intoSignBundle(plainText, ...), so a marshal failure gets signed and sealed as empty bytes.Not touched
polyamide/conn/bind_std.go:255has the same SA4006 shape (numMsgs), butpolyamide/is a vendored wireguard-go fork, so it seemed better left to an upstream sync than changed here.Verification
go build ./...andGOOS=darwin go build ./...both pass,go test ./core/... ./state/...is green before and after,gofmt -lis clean on all four files, and first-party SA4006 goes from 4 to 0.I did not add a test. The UAPI path needs a real unix socket under the system socket directory, so a test would either need root or assert a failure that only reproduces without it, and would flip depending on how CI runs. If there is a pattern you use for the sys_* files, point me at it and I will add one.