From 8a3b1dff73e52bd30f3edc635b7dd1d0c7ae82c9 Mon Sep 17 00:00:00 2001
From: Graham <gpe@openrs2.org>
Date: Wed, 29 Dec 2021 09:46:05 +0000
Subject: [PATCH] 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>
---
 .../protocol/js5/Js5ResponseEncoder.kt        | 68 ++++++++++++++++++-
 .../protocol/js5/Js5ResponseEncoderTest.kt    | 19 ++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/protocol/src/main/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoder.kt b/protocol/src/main/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoder.kt
index be7342e99f..82306c8f2c 100644
--- a/protocol/src/main/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoder.kt
+++ b/protocol/src/main/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoder.kt
@@ -34,7 +34,73 @@ public object Js5ResponseEncoder : MessageToByteEncoder<Js5Response>(Js5Response
 
     override fun allocateBuffer(ctx: ChannelHandlerContext, msg: Js5Response, preferDirect: Boolean): ByteBuf {
         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) {
             ctx.alloc().ioBuffer(len, len)
diff --git a/protocol/src/test/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoderTest.kt b/protocol/src/test/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoderTest.kt
index 0c8a21f0c1..5141546a41 100644
--- a/protocol/src/test/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoderTest.kt
+++ b/protocol/src/test/kotlin/org/openrs2/protocol/js5/Js5ResponseEncoderTest.kt
@@ -1,6 +1,7 @@
 package org.openrs2.protocol.js5
 
 import io.netty.buffer.ByteBuf
+import io.netty.buffer.ByteBufAllocator
 import io.netty.buffer.Unpooled
 import io.netty.channel.embedded.EmbeddedChannel
 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) {
         val channel = EmbeddedChannel(Js5ResponseEncoder)