msgpack-java icon indicating copy to clipboard operation
msgpack-java copied to clipboard

MessageBuffer allows unsafe out-of-bounds reads and writes

Open panicbit opened this issue 5 years ago • 2 comments

The get and put family of functions on MessageBuffer use unsafe memory accesses and are missing bounds checks. They can thus can read and write out of bounds.

E.g. in the following example getInt either causes a segfault or returns an undefined value:

val unpacker = MessagePack.newDefaultUnpacker(ByteArray(0))
val buffer = unpacker.readPayloadAsReference(0)
val value = buffer.getInt(9000000)

println("Value: $value")

Since this unsafety is exposed publicly and is not documented, it can be quite dangerous. If the ability to skip the bounds check is an intended feature, then I'd suggest to name the methods accordingly (e.g. giving them an "unsafe" prefix). Having a set of functions that does bounds checking by default probably does not hurt either.

panicbit avatar Dec 24 '20 16:12 panicbit

@xerial Can you take a look at this?

komamitsu avatar Feb 14 '21 14:02 komamitsu

@panicbit Thanks for the report. Although this method has been exposed, we have no intention to let normal users who don't have the internal implementation knowledge of msgpack-java to rely on this functionality. As this method is already used in some of our dependencies, renaming this method to readUnsafeXXX cannot be our option. And also, adding boundary checks to MessageBuffer methods might also have non-negligible performance overhead, so let us just fix the documentation as in #546

xerial avatar Feb 14 '21 22:02 xerial