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.

Sign up for GitHub

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

Jump to bottom

Websockets, Inflate: deflatedBytes is not empty when inflate is called in MessageInflater. ("java.lang.IllegalArgumentException: Failed requirement." Exception) #8551

Open
Fischiii opened this issue Oct 11, 2024 · 5 comments · May be fixed by #8610
Open

Websockets, Inflate: deflatedBytes is not empty when inflate is called in MessageInflater. ("java.lang.IllegalArgumentException: Failed requirement." Exception) #8551

Fischiii opened this issue Oct 11, 2024 · 5 comments · May be fixed by #8610
Labels
bug Bug in existing code needs info More information needed from reporter

Comments

@Fischiii
Copy link

When using binary data over WebSockets we trigger the require statement in the inflate method of the MessageInflater. This happens with release 5.0.0-alpha.12 and 5.0.0.alpha.14. We are using OKhttp on Android.

This only seems to happen with a certain Package size and composition. We attached the packet we used to replicate this a 100% of the time. This is the package before it is being deflated. We can reproduce this issue with multiple server setups. If we return the package below on a server that understands per message deflate we will get the Exception every time:

bytestream.txt

The failure is always that the inflate method is called but deflatedBytes.size is > 0. Since it is a require call that it must be 0 we fail here.

Image

We assume that the actual issue is on the inflate before the actual failure as this is apparently "finished" while still leaving 4 bytes in the stream.

The last inflate call in WebSocketReader before we trigger the require condition

Image

Before the last readOrInflate call before we trigger the require condition.

Image

In the last call of readOrInflateBefore we trigger the require condition.

Image

The end of the last inflate call before we trigger the require condition. Note that deflatedBytes size is 4 here which is the size we will trigger the require condition with.

Image

Image

We haven not been able to pin down the why this happens exactly, but we can we can reproduce this a 100% of the time so we can easily provide further information if needed.
@Fischiii Fischiii added the bug Bug in existing code label Oct 11, 2024
@yschimke
Copy link
Collaborator

yschimke commented Oct 12, 2024 •
edited
Loading

cc @swankjesse

I wonder whether it's triggering the codepath changed in #8078 ? We avoid infinite loop, but leave bytes in the buffer?

@Fischiii
Copy link
Author

@yschimke I just want to add that we have been using the version with the fix successfully for some time. Just one litte change in the provided payload and the package works. We must be running into some edge case here.

@yschimke
Copy link
Collaborator

I can't reproduce this exception, even if there are additional bytes left in a completed inflation, it's the inflater is either still finished, or reset.

I tried with https://github.com/yschimke/okhttp/pull/new/test_inflator without luck

I'm probably doing something wrong and unfamiliar with the code, but I can't inflate that input either.

[size=141991 hex=1aa3d5080a9f01122433353233336163322d306135632d343834372d386630342d65623966373765366661323641000000000000424078bebbc5a3a2328101cd…]

java.util.zip.DataFormatException: invalid literal/length code
java.io.IOException: java.util.zip.DataFormatException: invalid literal/length code
	at okio.InflaterSource.readOrInflate(InflaterSource.kt:92)
	at okhttp3.internal.ws.MessageInflater.inflate(MessageInflater.kt:56)
	at okhttp3.internal.ws.MessageDeflaterInflaterTest.inflate(MessageDeflaterInflaterTest.kt:167)
	at okhttp3.internal.ws.MessageDeflaterInflaterTest.illegal argument exception after bad packet(MessageDeflaterInflaterTest.kt:62)

Are you able to provide a reproduction? I guess specifically on Android?

@Fischiii
Copy link
Author

Fischiii commented Nov 4, 2024 •
edited
Loading

Hi @yschimke, sorry for the late reply. I will try to respond right away in the future.

I want to reiterate that the package provided is the direct package used by the server, not yet the deflated one. So to reproduce in an integration test we would need to put that package to a server that supports per message deflate. I will try to extract the deflated package and provide this as well.

Yes Android is correct.

@yschimke yschimke added the needs info More information needed from reporter label Nov 4, 2024
@Fischiii Fischiii mentioned this issue Nov 19, 2024
Open
@Fischiii
Copy link
Author

Fischiii commented Nov 19, 2024 •
edited
Loading

@yschimke added tests to reproduce the issue in:
#8582

Sorry for the big messages in the Tests, but I could not reproduce this issue with a shorter subset of the message.

swankjesse pushed a commit that referenced this issue Dec 6, 2024
@squarejesse
Discard trailing data after a deflated message
23321ca 
We can recover from this and it seems like the least worst of our
options.

Closes: #8551
@swankjesse swankjesse linked a pull request Dec 6, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Bug in existing code needs info More information needed from reporter
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Discard trailing data after a deflated message square/okhttp
2 participants
@yschimke @Fischiii