SA5011 false positive after (*testing.T).Fatalf

[tamird]

These occur after updating to v0.1.1. Two instances:

https://github.com/google/gvisor/blob/fcc2468db573fec37c91236f805f7fa0e46e4492/pkg/sentry/fs/fdpipe/pipe_opener_test.go#L349-L366

https://github.com/google/gvisor/blob/fcc2468db573fec37c91236f805f7fa0e46e4492/pkg/tcpip/tests/integration/link_resolution_test.go#L400-L429

Raw results:

• https://storage.googleapis.com/gvisor-buildkite/7bc1c9fc-1448-41d1-9c08-64d76ea8d46e/e31744a4-2c50-4a77-a692-21323350aee6/home/agent/.cache/bazel/_bazel_agent/843778434294ab14ec56b0cf0ea4c952/execroot/__main__/bazel-out/k8-fastbuild/testlogs/pkg/sentry/fs/fdpipe/fdpipe_test_nogo/test.log
• https://storage.googleapis.com/gvisor-buildkite/7bc1c9fc-1448-41d1-9c08-64d76ea8d46e/e31744a4-2c50-4a77-a692-21323350aee6/home/agent/.cache/bazel/_bazel_agent/843778434294ab14ec56b0cf0ea4c952/execroot/__main__/bazel-out/k8-fastbuild/testlogs/pkg/tcpip/tests/integration/integration_test_nogo/test.log

cc @ghanan94

[dominikh]

I cannot reproduce the problem by running Staticcheck proper. How are you running Staticcheck?

[dominikh]

Of course I find it hard to reproduce when this happens:

$ go build ./pkg/sentry/fs/fdpipe 
go: finding module for package gvisor.dev/gvisor/pkg/sentry/arch/registers_go_proto
go: finding module for package gvisor.dev/gvisor/pkg/metric/metric_go_proto
go: finding module for package gvisor.dev/gvisor/pkg/eventchannel/eventchannel_go_proto
pkg/eventchannel/event.go:29:2: no matching versions for query "latest"
pkg/metric/metric.go:26:2: no matching versions for query "latest"
pkg/sentry/arch/arch_x86.go:27:2: no matching versions for query "latest"
pkg/linewriter/linewriter.go:21:2: found packages sync (aliases.go) and seqatomic (generic_atomicptr_unsafe.go) in /tmp/gvisor/pkg/sync

[tamird]

Ah, that's because that project normally builds with bazel. The project has a synthetic branch called "go" that is compatible with the standard go tools, but all the tests are stripped. I was able to run staticcheck over it:

git clone https://github.com/google/gvisor.git --branch go
cd gvisor
go get honnef.co/go/tools/cmd/staticcheck                                                                                   
go: downloading honnef.co/go/tools v0.1.1                                                                                                                                                                  
go: found honnef.co/go/tools/cmd/staticcheck in honnef.co/go/tools v0.1.1                                                                                                                                  
go: downloading golang.org/x/tools v0.1.0                                                                                                                                                                  
go: downloading github.com/BurntSushi/toml v0.3.1                                                                                                                                                          
go: downloading golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4                                                                                                                                        
go: downloading golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1                                                                                                                                    
go: downloading golang.org/x/mod v0.4.0                                              
git checkout origin/master -- pkg/tcpip/{checker,faketime,tests/integration}
staticcheck ./pkg/tcpip/tests/integration/...

However, that doesn't reproduce the problem. I'm working on updating the google-internal dependency on go-tools, will get back when I've done so.

[dominikh]

I take it then that you're using nogo to run Staticcheck's analyzes? My suspicion is that nogo isn't running the analyzes on any of the standard library, which precludes us from annotating (*testing.T).Fatalfas a function that aborts control flow.

Assuming I am right about that, we could work around that, but I'd much prefer if nogo fixed their implementation of an analysis runner.

[tamird]

Totally possible -- I'm not entirely sure. @amscanne may know more about this, having set it up.

[tamird]

The plot thickens. Having updated the internal version to 0.1.1, I hit this panic:

panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
google3/third_party/golang/dominikh/tools/analysis/facts/nilness/nilness.impl.func2(0x5589416d1358, 0xc000d82c60, 0x1)
        third_party/golang/dominikh/tools/analysis/facts/nilness/nilness.go:161 +0x1205
google3/third_party/golang/dominikh/tools/analysis/facts/nilness/nilness.impl(0xc001a10f70, 0xc00160ed80, 0xc000371890, 0xc001a9f1b6, 0x2, 0x2)
        third_party/golang/dominikh/tools/analysis/facts/nilness/nilness.go:239 +0x3a4
google3/third_party/golang/dominikh/tools/analysis/facts/nilness/nilness.run(0xc001a10f70, 0xc00225ddb8, 0xc000371a20, 0xc0011092a8, 0x0)
        third_party/golang/dominikh/tools/analysis/facts/nilness/nilness.go:66 +0x152
google3/third_party/gvisor/tools/nogo/nogo.CheckPackage.func1(0x5589419942c0, 0x0, 0x0)
        third_party/gvisor/tools/nogo/nogo.go:448 +0x567
google3/third_party/gvisor/tools/nogo/nogo.CheckPackage.func1(0xc0002de0d0, 0x0, 0x0)
        third_party/gvisor/tools/nogo/nogo.go:416 +0x133
google3/third_party/gvisor/tools/nogo/nogo.CheckPackage(0xc00013c3c0, 0xc000154000, 0x7a, 0xe0, 0x0, 0xc00013c3c0, 0x22, 0xc000112aa0, 0xc0001b3f78, 0x558940f20bec, ...)
        third_party/gvisor/tools/nogo/nogo.go:466 +0x7e9
main.main()
        third_party/gvisor/tools/nogo/check/main.go:76 +0x169

[tamird]

FWIW it doesn't look as though we're actually using the real nogo, rather a homegrown solution of the same name https://github.com/google/gvisor/blob/master/tools/nogo/nogo.go.

[dominikh]

I'll try and reproduce the issue(s) with that nogo. Give me a day or two.

[tamird]

I did a bit of digging; oddly I see this:

• nilness is analyzing this function: https://github.com/google/gvisor/blob/f52f0101bb651924b404e7843b1e383b6845a107/pkg/sentry/vfs/file_description_impl_util.go#L312
• it asks the pass for facts:

  go-tools/analysis/facts/nilness/nilness.go

  Lines 102 to 104 in 3ce2d47

  	if fact := new(neverReturnsNilFact); pass.ImportObjectFact(fn.Object(), fact) {
  	return fact.Rets
  	}

• facts come back with: fact=never returns nil: [never] which makes no sense because the function has two return values
• panic

FWIW the version of golang/tools appears to be 0.1.0: https://github.com/golang/tools/releases/tag/v0.1.0.

[tamird]

OK, I think the function that's confusing nilness is https://github.com/google/gvisor/blob/fe63db2e96069140a3e02a0dc7b7e28af9b463db/pkg/sentry/fsimpl/kernfs/dynamic_bytes_file.go#L106.

DynamicBytesFD embeds a vfs.DynamicBytesFileDescriptionImpl and delegates its Read function to it; the result is that the embedder's function appears to have only one return. There's an odd interaction with {Import,Export}ObjectFact that I don't fully understand; stubbing out the call to ExportObjectFact causes things to work again. I instrumented the {Import,Export} sites thinking it operated on object identity, but saw this:

exporting 0xc0008bd590: (*gvisor/pkg/sentry/fsimpl/kernfs/kernfs.DynamicBytesFile).SetStat
imported 0xc000801c70: (*gvisor/pkg/sentry/vfs/vfs.DynamicBytesFileDescriptionImpl).Read
exporting 0xc0008bd6d0: (*gvisor/pkg/sentry/fsimpl/kernfs/kernfs.DynamicBytesFD).Read // this one happens during panic

so the only fact exported belongs to SetStat, but then a fact about Read is imported. Any idea why that might happen? As mentioned, removing the ExportObjectFact call fixes things.

[tamird]

The previous comment may be a red herring - I don't know if there's caching going on, where the exported fact was cached in some previous package build and that's why it's not shown in that output.