- Notifications
You must be signed in to change notification settings - Fork9.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Discard trailing data after a deflated message#8610
base:master
Are you sure you want to change the base?
Conversation
We can recover from this and it seems like the least worst of ouroptions.Closes:#8551
| if (inflater.bytesRead < totalBytesToRead) { | ||
| deflatedBytes.clear() | ||
| inflaterSource.close() | ||
| this.inflaterSource = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I’m gonna claim it’s really weird if you have a stream-terminating message followed by another message andnoContextTakeover = false. But perhaps I should write a test for this? I’m not exactly sure what reasonable behavior is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also - I think the server in the case we’re addressing here is probably buggy. We might be better to follow@Fischiii’s lead and just reject the bogus data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think that was my change to make his PR pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@swankjesse@yschimke in case it helps. The server used is a Kestrel (.net version 7) with the WebsocketSharp.Standard2 Framework for the WebwSocket connection. Data transferred is binary form Protobuffer.
| @@ -136,6 +139,41 @@ internal class MessageDeflaterInflaterTest { | |||
| assertThat(buffer.readUtf8()).isEqualTo("Hello inflation!") | |||
| } | |||
| /** | |||
| * It's possible a self-terminating deflated message will contain trailing data that won't be | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it worth tying this back to the padding bytes in the spec?
We can recover from this and it seems like the least worst of our options.
Closes:#8551