Fix off-by-one error in JS5 response buffer capacity calculation

In some cases the capacity would be one byte greater than necessary.

I've added a comment explaining why the calculation works and a unit
test that ensures it is calculated correctly for all lengths between 1
and 511*3+1 bytes.

Signed-off-by: Graham <gpe@openrs2.org>
bzip2
Graham 3 years ago
parent 469fe2eecc
commit 8a3b1dff73
  1. 68
      protocol/src/main/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoder.kt
  2. 19
      protocol/src/test/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoderTest.kt

@ -34,7 +34,73 @@ public object Js5ResponseEncoder : MessageToByteEncoder<Js5Response>(Js5Response
override fun allocateBuffer(ctx: ChannelHandlerContext, msg: Js5Response, preferDirect: Boolean): ByteBuf { override fun allocateBuffer(ctx: ChannelHandlerContext, msg: Js5Response, preferDirect: Boolean): ByteBuf {
val dataLen = msg.data.readableBytes() val dataLen = msg.data.readableBytes()
val len = 3 + dataLen + (2 + dataLen) / 511
/*
* The naive code to estimate the length is:
*
* var len = 3 // for the archive/group header
* len += dataLen // for the data itself
*
* // first 0xFF marker
* if (dataLen > 509) { // compression byte plus the first 508 bytes of data
* len++
* dataLen -= 509
*
* // remaining 0xFF markers
* while (dataLen > 511) {
* len++
* dataLen -= 511
* }
* }
*
* We can simplify this by combining the if and while loop by adding an
* extra 2 bytes to dataLen, which ensures we add the first marker
* after 509 bytes (511-2) and then all subsequent markers after 511
* bytes:
*
* var len = 3
* len += dataLen
*
* dataLen += 2
*
* while (dataLen > 511) {
* len++
* dataLen -= 511
* }
*
* The while loop can be replaced with division:
*
* var len = 3
* len += dataLen
* len += divide(dataLen + 2, 511)
*
* divide() is almost but not quite standard division. We want
* divide(x, 511) to produce 0 if x=511, and only produce 1 if x=512.
* Similarly, it should produce 1 if x=1022, and only produce 1 if
* x=1023, and so on.
*
* This can be achieved by implementing divide(x, y) as
* ((x + (y - 1)) / y) - 1. So in our case, where y=511,
* ((x + 510) / y) - 1.
*
* Combined with the above, our length calculation becomes:
*
* var len = 3
* len += dataLen
* len += (dataLen + 2 + 510) / 511 - 1
*
* which we can simplify to:
*
* val len = 2 + dataLen + (512 + dataLen) / 511
*
* As this is confusing, testAllocateBuffer() has a test to ensure the
* length calculation is correct for all dataLens between 1 and 1534
* inclusive, which covers all lengths for 1, 2 and 3 block responses
* completely, and one length for a 4 block responses (as a boundary
* check).
*/
val len = 2 + dataLen + (512 + dataLen) / 511
return if (preferDirect) { return if (preferDirect) {
ctx.alloc().ioBuffer(len, len) ctx.alloc().ioBuffer(len, len)

@ -1,6 +1,7 @@
package org.openrs2.protocol.js5 package org.openrs2.protocol.js5
import io.netty.buffer.ByteBuf import io.netty.buffer.ByteBuf
import io.netty.buffer.ByteBufAllocator
import io.netty.buffer.Unpooled import io.netty.buffer.Unpooled
import io.netty.channel.embedded.EmbeddedChannel import io.netty.channel.embedded.EmbeddedChannel
import io.netty.handler.codec.EncoderException import io.netty.handler.codec.EncoderException
@ -40,6 +41,24 @@ class Js5ResponseEncoderTest {
} }
} }
@Test
fun testAllocateBuffer() {
val channel = EmbeddedChannel(Js5ResponseEncoder)
for (len in 1..1534) {
ByteBufAllocator.DEFAULT.buffer(len, len).use { data ->
data.writeZero(len)
channel.writeOutbound(Js5Response(true, 2, 3, data.retain()))
}
channel.readOutbound<ByteBuf>().use { actual ->
// check that allocateBuffer estimates the required capacity exactly
assertEquals(actual.writerIndex(), actual.capacity())
}
}
}
private fun testEncode(container: String, encoded: String, prefetch: Boolean) { private fun testEncode(container: String, encoded: String, prefetch: Boolean) {
val channel = EmbeddedChannel(Js5ResponseEncoder) val channel = EmbeddedChannel(Js5ResponseEncoder)

Loading…
Cancel
Save