From a88e0b6cb17935fd244725e067b97ab7ac59597c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Mon, 13 Feb 2023 22:22:39 +0100 Subject: [PATCH] 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 --- CHANGELOG.md | 4 ++- .../internal/MsgPackDecoder.kt | 30 +++++++++---------- .../msgpack/MsgPackDecoderTest.kt | 6 ++-- .../kotlinx/serialization/msgpack/TestData.kt | 6 ++-- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df82951..9f4ca1d 100644 --- 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/serialization-msgpack/src/commonMain/kotlin/com.ensarsarajcic.kotlinx.serialization.msgpack/internal/MsgPackDecoder.kt b/serialization-msgpack/src/commonMain/kotlin/com.ensarsarajcic.kotlinx.serialization.msgpack/internal/MsgPackDecoder.kt index e36203f..c777576 100644 --- a/serialization-msgpack/src/commonMain/kotlin/com.ensarsarajcic.kotlinx.serialization.msgpack/internal/MsgPackDecoder.kt +++ b/serialization-msgpack/src/commonMain/kotlin/com.ensarsarajcic.kotlinx.serialization.msgpack/internal/MsgPackDecoder.kt @@ -27,17 +27,15 @@ internal class BasicMsgPackDecoder( val inlineDecoders: Map Decoder> = mapOf() ) : AbstractDecoder(), MsgPackTypeDecoder { - private val depthStack: ArrayDeque = 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/serialization-msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/MsgPackDecoderTest.kt b/serialization-msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/MsgPackDecoderTest.kt index 5d21225..2d83e36 100644 --- a/serialization-msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/MsgPackDecoderTest.kt +++ b/serialization-msgpack/src/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 testPairs(decodeFunction: MsgPackDecoder.() -> RESULT, vararg pairs: Pair) { pairs.forEach { (input, result) -> MsgPackDecoder(BasicMsgPackDecoder(MsgPackConfiguration.default, SerializersModule {}, input.hexStringToByteArray().toMsgPackBuffer())).also { - assertEquals(result, it.decodeFunction()) - } + assertEquals(result, it.decodeFunction()) + } } } } diff --git a/serialization-msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/TestData.kt b/serialization-msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/TestData.kt index a75a3ce..a8c873b 100644 --- a/serialization-msgpack/src/commonTest/kotlin/com/ensarsarajcic/kotlinx/serialization/msgpack/TestData.kt +++ b/serialization-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)), ) }