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

Discard trailing data after a deflated message #8610

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions okhttp/src/main/kotlin/okhttp3/internal/ws/MessageInflater.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,22 @@ class MessageInflater(
) : Closeable {
private val deflatedBytes = Buffer()

private val inflater =
Inflater(
// nowrap (omits zlib header):
true,
)

private val inflaterSource = InflaterSource(deflatedBytes, inflater)
// Lazily-created.
private var inflater: Inflater? = null
private var inflaterSource: InflaterSource? = null

/** Inflates [buffer] in place as described in RFC 7692 section 7.2.2. */
@Throws(IOException::class)
fun inflate(buffer: Buffer) {
require(deflatedBytes.size == 0L)

val inflater =
this.inflater
?: Inflater(true).also { this.inflater = it }
val inflaterSource =
this.inflaterSource
?: InflaterSource(deflatedBytes, inflater).also { this.inflaterSource = it }

if (noContextTakeover) {
inflater.reset()
}
Expand All @@ -55,8 +58,21 @@ class MessageInflater(
do {
inflaterSource.readOrInflate(buffer, Long.MAX_VALUE)
} while (inflater.bytesRead < totalBytesToRead && !inflater.finished())

// The inflater data was self-terminated and there's unexpected trailing data. Tear it all down
// so we don't leak that data into the input of the next message.
if (inflater.bytesRead < totalBytesToRead) {
deflatedBytes.clear()
inflaterSource.close()
this.inflaterSource = null
Copy link
Collaborator Author

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 and noContextTakeover = false. But perhaps I should write a test for this? I’m not exactly sure what reasonable behavior is.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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

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.

this.inflater = null
}
}

@Throws(IOException::class)
override fun close() = inflaterSource.close()
override fun close() {
inflaterSource?.close()
inflaterSource = null
inflater = null
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@ import assertk.assertThat
import assertk.assertions.isEqualTo
import assertk.assertions.isLessThan
import java.io.EOFException
import java.util.zip.Deflater
import kotlin.test.assertFailsWith
import okhttp3.TestUtil.fragmentBuffer
import okio.Buffer
import okio.ByteString
import okio.ByteString.Companion.decodeHex
import okio.ByteString.Companion.encodeUtf8
import okio.DeflaterSink
import okio.use
import org.junit.jupiter.api.Test

internal class MessageDeflaterInflaterTest {
Expand Down Expand Up @@ -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
Copy link
Collaborator

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?

* processed during inflation. If this happens, we need to either reject the message or discard
* the unreachable data. We choose to discard it!
*
* In practice this could happen if the encoder doesn't strip the [0x00, 0x00, 0xff, 0xff] suffix
* and that ends up repeated.
*
* https://github.com/square/okhttp/issues/8551
*/
@Test
fun `deflated data has too many bytes`() {
val inflater = MessageInflater(true)
val buffer = Buffer()

val message1 = "hello".encodeUtf8()
val message2 = "hello 2".encodeUtf8()

DeflaterSink(buffer, Deflater(Deflater.DEFAULT_COMPRESSION, true)).use { sink ->
sink.write(Buffer().write(message1), message1.size.toLong())
}
buffer.writeByte(0x00)
// Trailing data. We use the Okio segment size to make sure it's still in the input buffer.
buffer.write(ByteArray(8192))
inflater.inflate(buffer)
assertThat(buffer.readByteString()).isEqualTo(message1)

DeflaterSink(buffer, Deflater(Deflater.DEFAULT_COMPRESSION, true)).use { sink ->
sink.write(Buffer().write(message2), message2.size.toLong())
}
buffer.writeByte(0x00)
inflater.inflate(buffer)
assertThat(buffer.readByteString()).isEqualTo(message2)
}

private fun MessageDeflater.deflate(byteString: ByteString): ByteString {
val buffer = Buffer()
buffer.write(byteString)
Expand Down
Loading