Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crashes in s3file.go #14

Closed
wants to merge 4 commits into from
Closed

Fix crashes in s3file.go #14

wants to merge 4 commits into from

Conversation

tatatodd
Copy link

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
Copy link
Contributor

jcharum commented Apr 30, 2020

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
Copy link
Contributor

jcharum commented Apr 30, 2020

I think #17 should cover this. I think the other call to newInfo is defunct (see #16 (comment)), so we'll ultimately just remove that case.

@tatatodd
Copy link
Author

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

@tatatodd tatatodd closed this Apr 30, 2020
jcharum added a commit that referenced this pull request May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants