fix: prevent overflow with ignoreUnknownKeys

If `ignoreUnknownKeys` flag is on, reading could overflow to further structures and cause issues when these structures need to be parsed. This closes #87 This fixes #82
esensar · Feb 13, 2023 · a88e0b6 · a88e0b6
1 parent c2fafa2
commit a88e0b6
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 All notable changes to this project will be documented in this file. This change log follows the conventions of [keepachangelog.com](http://keepachangelog.com/).
 
 ## [Unreleased]
+- Fixed `ignoreUnknownKeys` behavior in nested structures ([#82][i82] and [#87][p87])
 
 ## [0.5.3] - 2022-10-23
 - Fixed `strictTypes` flag to allow all number conversions and not just different precisions ([#81][i81])
@@ -107,7 +108,7 @@ MsgPack.default.encodeToByteArray(...)
 [0.5.0]: https://github.com/esensar/kotlinx-serialization-msgpack/compare/0.4.4...0.5.0
 [0.5.1]: https://github.com/esensar/kotlinx-serialization-msgpack/compare/0.5.0...0.5.1
 [0.5.2]: https://github.com/esensar/kotlinx-serialization-msgpack/compare/0.5.1...0.5.2
-[0.5.2]: https://github.com/esensar/kotlinx-serialization-msgpack/compare/0.5.2...0.5.3
+[0.5.3]: https://github.com/esensar/kotlinx-serialization-msgpack/compare/0.5.2...0.5.3
 [i6]: https://github.com/esensar/kotlinx-serialization-msgpack/issues/6
 [i9]: https://github.com/esensar/kotlinx-serialization-msgpack/issues/9
 [i10]: https://github.com/esensar/kotlinx-serialization-msgpack/issues/10
@@ -129,3 +130,4 @@ MsgPack.default.encodeToByteArray(...)
 [i81]: https://github.com/esensar/kotlinx-serialization-msgpack/issues/81
 [i82]: https://github.com/esensar/kotlinx-serialization-msgpack/issues/82
 [p40]: https://github.com/esensar/kotlinx-serialization-msgpack/pull/40
+[p87]: https://github.com/esensar/kotlinx-serialization-msgpack/pull/87
diff --git a/...monMain/kotlin/com.ensarsarajcic.kotlinx.serialization.msgpack/internal/MsgPackDecoder.kt b/...monMain/kotlin/com.ensarsarajcic.kotlinx.serialization.msgpack/internal/MsgPackDecoder.kt
@@ -27,17 +27,15 @@ internal class BasicMsgPackDecoder(
     val inlineDecoders: Map<SerialDescriptor, (InlineDecoderHelper) -> Decoder> = mapOf()
 ) : AbstractDecoder(), MsgPackTypeDecoder {
 
-    private val depthStack: ArrayDeque<Unit> = ArrayDeque()
-
-    override fun decodeElementIndex(descriptor: SerialDescriptor): Int {
+    internal fun decodeElementIndexInternal(descriptor: SerialDescriptor): Int {
         if (descriptor.kind in arrayOf(StructureKind.CLASS, StructureKind.OBJECT)) {
             val next = dataBuffer.peekSafely()
             if (next != null && MsgPackType.String.isString(next)) {
                 val fieldName = kotlin.runCatching { decodeString() }.getOrNull() ?: return CompositeDecoder.UNKNOWN_NAME
                 val index = descriptor.getElementIndex(fieldName)
-                return if (index == CompositeDecoder.UNKNOWN_NAME && configuration.ignoreUnknownKeys && depthStack.isEmpty()) {
+                return if (index == CompositeDecoder.UNKNOWN_NAME && configuration.ignoreUnknownKeys) {
                     MsgPackNullableDynamicSerializer.deserialize(this)
-                    decodeElementIndex(descriptor)
+                    index
                 } else {
                     index
                 }
@@ -47,6 +45,13 @@ internal class BasicMsgPackDecoder(
         }
         return 0
     }
+    override fun decodeElementIndex(descriptor: SerialDescriptor): Int {
+        val result = decodeElementIndexInternal(descriptor)
+        if (result == CompositeDecoder.UNKNOWN_NAME && configuration.ignoreUnknownKeys) {
+            return decodeElementIndex(descriptor)
+        }
+        return result
+    }
 
     override fun decodeSequentially(): Boolean = true
 
@@ -184,19 +189,12 @@ internal class BasicMsgPackDecoder(
                 return ExtensionTypeDecoder(this)
             }
 
-            depthStack.addFirst(Unit)
-            // Handle extension types as arrays
             val size = decodeCollectionSize(descriptor)
-            return ClassMsgPackDecoder(this, size)
+            return ClassMsgPackDecoder(this, configuration, size)
         }
         return this
     }
 
-    override fun endStructure(descriptor: SerialDescriptor) {
-        super.endStructure(descriptor)
-        depthStack.removeFirstOrNull()
-    }
-
     override fun peekNextType(): Byte {
         return dataBuffer.peek()
     }
@@ -210,6 +208,7 @@ internal class MsgPackDecoder(
 
 internal class ClassMsgPackDecoder(
     private val basicMsgPackDecoder: BasicMsgPackDecoder,
+    private val configuration: MsgPackConfiguration,
     private val size: Int
 ) : Decoder by basicMsgPackDecoder, CompositeDecoder by basicMsgPackDecoder, MsgPackTypeDecoder by basicMsgPackDecoder {
     override val serializersModule: SerializersModule = basicMsgPackDecoder.serializersModule
@@ -218,10 +217,9 @@ internal class ClassMsgPackDecoder(
 
     override fun decodeElementIndex(descriptor: SerialDescriptor): Int {
         if (decodedElements >= size) return CompositeDecoder.DECODE_DONE
-        val result = basicMsgPackDecoder.decodeElementIndex(descriptor)
+        val result = basicMsgPackDecoder.decodeElementIndexInternal(descriptor)
         if (result != CompositeDecoder.DECODE_DONE) decodedElements++
-        return if (result == CompositeDecoder.UNKNOWN_NAME) {
-            MsgPackNullableDynamicSerializer.deserialize(this)
+        return if (result == CompositeDecoder.UNKNOWN_NAME && configuration.ignoreUnknownKeys) {
             decodeElementIndex(descriptor)
         } else {
             result

diff --git a/...c/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/MsgPackDecoderTest.kt b/...c/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/MsgPackDecoderTest.kt
@@ -154,7 +154,7 @@ internal class MsgPackDecoderTest {
     @Test
     fun testSampleClassWithNestedValueAndMissingKeys() {
         TestData.nestedSampleClassWithMissingValue.forEach { (input, result) ->
-            val decoder = BasicMsgPackDecoder(MsgPackConfiguration.default, SerializersModule {}, input.hexStringToByteArray().toMsgPackBuffer())
+            val decoder = BasicMsgPackDecoder(MsgPackConfiguration.default.copy(ignoreUnknownKeys = true), SerializersModule {}, input.hexStringToByteArray().toMsgPackBuffer())
             val serializer = TestData.SampleClassWithNestedClass.serializer()
             assertEquals(result, serializer.deserialize(decoder))
         }
@@ -199,8 +199,8 @@ internal class MsgPackDecoderTest {
     private fun <RESULT> testPairs(decodeFunction: MsgPackDecoder.() -> RESULT, vararg pairs: Pair<String, RESULT>) {
         pairs.forEach { (input, result) ->
             MsgPackDecoder(BasicMsgPackDecoder(MsgPackConfiguration.default, SerializersModule {}, input.hexStringToByteArray().toMsgPackBuffer())).also {
-            assertEquals(result, it.decodeFunction())
-        }
+                assertEquals(result, it.decodeFunction())
+            }
         }
     }
 }
diff --git a/...msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/TestData.kt b/...msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/TestData.kt
@@ -157,7 +157,8 @@ object TestData {
         val testString: String,
         val testInt: Int,
         val testBoolean: Boolean,
-        val testNested: NestedClass
+        val testNested: NestedClass,
+        val secondNested: NestedClass? = null
     ) {
         @Serializable
         data class NestedClass(
@@ -183,7 +184,8 @@ object TestData {
         "83a56669727374a5416c696365a67365636f6e64a3426f62a57468697264a454657374" to Triple("Alice", "Bob", "Test")
     )
     val nestedSampleClassWithMissingValue = arrayOf(
-        "84aa74657374537472696e67a3646566a774657374496e747bab74657374426f6f6c65616ec3aa746573744e657374656482aa74657374537472696e67a3646566ab74657374426f6f6c65616ec3" to SampleClassWithNestedClass("def", 123, true, SampleClassWithNestedClass.NestedClass(null))
+        "84aa74657374537472696e67a3646566a774657374496e747bab74657374426f6f6c65616ec3aa746573744e657374656482aa74657374537472696e67a3646566ab74657374426f6f6c65616ec3" to SampleClassWithNestedClass("def", 123, true, SampleClassWithNestedClass.NestedClass(null)),
+        "85aa74657374537472696e67a3646566a774657374496e747bab74657374426f6f6c65616ec3aa746573744e657374656483aa74657374537472696e67a3646566ab74657374426f6f6c65616ec3ae616e6f74686572556e6b6e6f776ea474657374ac7365636f6e644e657374656481ab74657374426f6f6c65616ec2" to SampleClassWithNestedClass("def", 123, true, SampleClassWithNestedClass.NestedClass(null), SampleClassWithNestedClass.NestedClass(null)),
     )
 }