grailbio / base

A collection of Go utility packages used by GRAIL's tools
Apache License 2.0
89 stars 25 forks source link

Fix crashes in s3file.go #14

Closed tatatodd closed 4 years ago

tatatodd commented 4 years ago

Fixes crashes like the following. I don't know exactly what conditions cause the AWS response to have nil values, but this fixes trivial nil pointer dereferences and seems better regardless.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xc55174]

goroutine 579033 [running]:
github.com/grailbio/base/file/s3file.newInfo(...)
/Users/toddwang/go/pkg/mod/github.com/grailbio/base@v0.0.7/file/s3file/s3file.go:726
github.com/grailbio/base/file/s3file.(*s3File).handleStat(0xc03ba87360, 0x2bd4760, 0xc03bd4d350, 0x2, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc03be145a0)
/Users/toddwang/go/pkg/mod/github.com/grailbio/base@v0.0.7/file/s3file/s3file.go:761 +0x424
github.com/grailbio/base/file/s3file.(*s3File).handleRequests(0xc03ba87360)
/Users/toddwang/go/pkg/mod/github.com/grailbio/base@v0.0.7/file/s3file/s3file.go:657 +0x1a7
created by github.com/grailbio/base/file/s3file.(*s3Impl).internalOpen
/Users/toddwang/go/pkg/mod/github.com/grailbio/base@v0.0.7/file/s3file/s3file.go:457 +0x1f2
jcharum commented 4 years ago

Thanks for the PR!

I looked into possible reasons why this might happen and ended up doing #16. Could you check if that eliminates your panic?

If not, could you create a reproducible case for us?

I want to avoid silently handling unexpected state, but I agree that we should defend against the AWS SDK behaving unexpectedly. Instead of transforming values silently, let's propagate errors. Users won't be surprised by surprisingly wrong stats, and we'll hopefully hear about unhandled SDK behavior.

What do you think?

jcharum commented 4 years ago

I think https://github.com/grailbio/base/pull/17 should cover this. I think the other call to newInfo is defunct (see https://github.com/grailbio/base/pull/16#issuecomment-622092442), so we'll ultimately just remove that case.

tatatodd commented 4 years ago

@jcharum Yup I agree #17 should cover this; I'll close this out. Thanks!