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

Proto: Catch pogs panic #2605

Merged
merged 1 commit into from
Apr 24, 2019
Merged

Proto: Catch pogs panic #2605

merged 1 commit into from
Apr 24, 2019

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Apr 23, 2019

Catch panics when deserializing capnp messages using pogs.
This could happen on malformed input, see capnproto/go-capnp#137

This change is Reviewable

@oncilla oncilla requested a review from scrye April 23, 2019 09:13
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment to the upstream issue (capnproto/go-capnp#137) in the commit message and in the code. I guess normally we shouldn't expect the library to panic?
So once this is fixed upstream can we remove this patch again?

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @scrye)

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the catching the panic is actually the right thing to do, even when the issue is fixed.

Our services should not panic because of malformed input.
Sure, panics should not cross package boundaries as per go convention. However, as this has shown, we cannot always rely on this.

I feel a lot more comfortable if we have a recover after calls to libraries that handle tainted input.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @scrye)

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the recover code, just to minimize risk of services crashing. Otherwise we need to perform a fuzzing check (which might be incomplete) every time we change the commit.

(Also related, historically we did not deploy adapters/API regression tests on top of third-party libraries, but cases like this show why in some cases it is useful to have them, especially for very complex libraries that deal with encoding/decoding).

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @scrye)

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Catch panics when deserializing capnp messages using pogs.
This could happen on malformed input, see
capnproto/go-capnp#137
@oncilla oncilla merged commit 988ba27 into scionproto:master Apr 24, 2019
@oncilla oncilla deleted the pub-catch-panic branch April 24, 2019 07:03
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.

3 participants