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

Panic when calling github.Event.ParsePayload() with nil RawPayload (artificial mock scenario) #743

Open
tleyden opened this Issue Oct 5, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@tleyden

tleyden commented Oct 5, 2017

I'm seeing the following panic in a unit test that uses mocks and doesn't set the event payload:

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

goroutine 56 [running]:
github.com/google/go-github/github.(*Event).ParsePayload(0xc4201a2840, 0x43, 0x0, 0xc4201c6460, 0x0)
	/Users/tleyden/go/src/github.com/google/go-github/github/activity_events.go:102 +0x93
github.com/tleyden/keynuker.GithubUserEventsScanner.discoverLeakerEmail(0x1acb660, 0xc4201a2800, 0xc4201a2840, 0x2, 0x2, 0x40, 0xc420199b60)
	/Users/tleyden/go/src/github.com/tleyden/keynuker/github_user_events_scanner.go:220 +0x43
github.com/tleyden/keynuker.GithubUserEventsScanner.scanAwsKeysForUser(0x1acb660, 0xc4201a2800, 0x1ace320, 0xc420016ef8, 0xc4201e8000, 0x17611d5, 0x4, 0x1769de4, 0x13, 0xc42019a690, ...)
	/Users/tleyden/go/src/github.com/tleyden/keynuker/github_user_events_scanner.go:182 +0xb26
github.com/tleyden/keynuker.GithubUserEventsScanner.ScanAwsKeys.func1(0xc4201a4720, 0xc42019a710, 0x1acb660, 0xc4201a2800, 0x1ace320, 0xc420016ef8, 0x17611d5, 0x4, 0x1769de4, 0x13, ...)
	/Users/tleyden/go/src/github.com/tleyden/keynuker/github_user_events_scanner.go:70 +0xe7
created by github.com/tleyden/keynuker.GithubUserEventsScanner.ScanAwsKeys
	/Users/tleyden/go/src/github.com/tleyden/keynuker/github_user_events_scanner.go:73 +0x19c
exit status 2
FAIL	github.com/tleyden/keynuker	0.026s

Steps to repro:

  1. Run this test: https://github.com/tleyden/keynuker/blob/3c2a1a334380f5e8ecabfcd5fb206d03c6bb58c4/github_user_events_scanner_test.go#L25-L164

The fix is trivial -- just check whether RawPayload is nil and return an error rather than panicking. I can provide a standalone test case if needed.

tleyden added a commit to tleyden/go-github that referenced this issue Oct 5, 2017

tleyden added a commit to tleyden/keynuker that referenced this issue Oct 5, 2017

Fix panic during test
Avoid panic described in google/go-github#743 by passing a non-nil RawPayload to the mock event

tleyden added a commit to tleyden/keynuker that referenced this issue Oct 5, 2017

Add benchmark for scanning, fix a unit test failure (#16)
* Convert from method -> func

* Add benchmark

30	  39429244 ns/op

* Fix panic during test

Avoid panic described in google/go-github#743 by passing a non-nil RawPayload to the mock event

* Test still fails, not extracting committer

* Fix fakePushEvent, test passing
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 5, 2017

Member

Can this happen in a production environment, or is it only possible to achieve the panic in a mock scenario?

Member

dmitshur commented Oct 5, 2017

Can this happen in a production environment, or is it only possible to achieve the panic in a mock scenario?

@tleyden

This comment has been minimized.

Show comment
Hide comment
@tleyden

tleyden Oct 5, 2017

@shurcooL it's a totally artificial bug, and can probably only happen in a mock scenario, so I completely understand if you don't want to bother with the checks.

tleyden commented Oct 5, 2017

@shurcooL it's a totally artificial bug, and can probably only happen in a mock scenario, so I completely understand if you don't want to bother with the checks.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 6, 2017

Member

Hmm, I wonder how to best deal with this...

On the one hand, perhaps ParsePayload should not panic on nil RawPayload, or it should be documented that it panics if RawPayload is nil.

On the other hand, there are probably many other ways to cause panics or undefined behavior, for example, if you set any of the fields in github.Client struct to nil... Should we try to address them all?

Member

dmitshur commented Oct 6, 2017

Hmm, I wonder how to best deal with this...

On the one hand, perhaps ParsePayload should not panic on nil RawPayload, or it should be documented that it panics if RawPayload is nil.

On the other hand, there are probably many other ways to cause panics or undefined behavior, for example, if you set any of the fields in github.Client struct to nil... Should we try to address them all?

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Oct 6, 2017

Collaborator

Hmmm... first taking the extreme case, I don't think we want to address them all.

Hypothetically speaking, if we do address them all, and plug every possible way that a panic could ever happen, then we have a package that never panics and always returns nice descriptive error messages.
OK, granted, that might indeed be cool.

However, chances are high that if the package panics, it is being used in a manner in which the collective authors did not intend (or there is a nasty bug... but we don't seem to be talking about that case here).

Ideally, this would be discovered immediately during development and not in a production environment where the code is being relied upon.

So it seems to me that in this specific case, it would be good to document the panic and not attempt to plug all panic possibilities. That being said, if someone felt strong enough to submit a PR that plugs the panic, I would probably LGTM that too. OK, so I just went in circles. But I would lean toward documenting it.

Collaborator

gmlewis commented Oct 6, 2017

Hmmm... first taking the extreme case, I don't think we want to address them all.

Hypothetically speaking, if we do address them all, and plug every possible way that a panic could ever happen, then we have a package that never panics and always returns nice descriptive error messages.
OK, granted, that might indeed be cool.

However, chances are high that if the package panics, it is being used in a manner in which the collective authors did not intend (or there is a nasty bug... but we don't seem to be talking about that case here).

Ideally, this would be discovered immediately during development and not in a production environment where the code is being relied upon.

So it seems to me that in this specific case, it would be good to document the panic and not attempt to plug all panic possibilities. That being said, if someone felt strong enough to submit a PR that plugs the panic, I would probably LGTM that too. OK, so I just went in circles. But I would lean toward documenting it.

@tleyden

This comment has been minimized.

Show comment
Hide comment
@tleyden

tleyden Oct 6, 2017

@shurcooL I asked about this on twitter .. sorry to move the conversation around!

tleyden commented Oct 6, 2017

@shurcooL I asked about this on twitter .. sorry to move the conversation around!

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 6, 2017

Member

I think I'd prefer documenting it too. That resolves the issue, and doesn't create churn.

As I replied on twitter:

The problem as I see is that it's a game of whack-a-mole. Why fix this only-possible-via-API-misuse panic but not 99 others?

E.g., https://play.golang.org/p/tg2Tvi7Ckx . There's a near infinite number of ways to cause panics by using APIs in ways they're not meant to be used...

Member

dmitshur commented Oct 6, 2017

I think I'd prefer documenting it too. That resolves the issue, and doesn't create churn.

As I replied on twitter:

The problem as I see is that it's a game of whack-a-mole. Why fix this only-possible-via-API-misuse panic but not 99 others?

E.g., https://play.golang.org/p/tg2Tvi7Ckx . There's a near infinite number of ways to cause panics by using APIs in ways they're not meant to be used...

@tleyden

This comment has been minimized.

Show comment
Hide comment
@tleyden

tleyden Oct 6, 2017

Forgetting about the mock / direct api call scenario, and thinking about the scenario where the github API didn't include that payload in a response, I think:

  • In this case, since the upstream API would be completely breaking the contract, it probably makes sense for the library to just panic.
  • Panicking would raise awareness. Eg, maybe the upstream API should be fixed to honor it’s contract, or the contract should be to updated to match reality.

and I agree with the whack-a-mole comment. At some point you have to make some assumptions (eg, the contract) about the data being returned from the upstream API, and if those are broken.. then all bets are off.

tleyden commented Oct 6, 2017

Forgetting about the mock / direct api call scenario, and thinking about the scenario where the github API didn't include that payload in a response, I think:

  • In this case, since the upstream API would be completely breaking the contract, it probably makes sense for the library to just panic.
  • Panicking would raise awareness. Eg, maybe the upstream API should be fixed to honor it’s contract, or the contract should be to updated to match reality.

and I agree with the whack-a-mole comment. At some point you have to make some assumptions (eg, the contract) about the data being returned from the upstream API, and if those are broken.. then all bets are off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment