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

Tests for inflator failure. #8582

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Fischiii
Copy link

#8551

Added tests to reproduce the issue.

Added one test with the broken deflated message:
"inflater left with deflated bytes"

Added another test with a similar, but working message:
"inflater left with deflated bytes working message"

The first test results in the exact same behaviour as we see within okhttp when we discovered the issue: #8551

@yschimke
Copy link
Collaborator

@Fischiii a big thanks for doing this.

@yschimke
Copy link
Collaborator

yschimke commented Nov 24, 2024

OK, thanks for the repro. See Fischiii#1 that you can merge into your PR.

But

  • I'm not clear if the server is sending additional bytes with per message deflate enabled.
    or
  • OkHttp isn't correctly accounting for the additional bytes at the end of a message

Context takeover discussed here https://websockets.readthedocs.io/en/stable/topics/compression.html#compression-parameters

Final block discussed here https://datatracker.ietf.org/doc/html/rfc7692#section-7.2.1

@swankjesse do you remember the algorithm here?

@Fischiii
Copy link
Author

@yschimke merged

@swankjesse
Copy link
Collaborator

I think I’ve isolated this?
https://github.com/square/okhttp/pull/8610/files

@yschimke
Copy link
Collaborator

yschimke commented Dec 6, 2024

@swankjesse since the spec specifically has additional padding data, are we sure it's not just a difference between the spec padding and what our vanilla compression expects?

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