Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MessageBuffer allows unsafe out-of-bounds reads and writes #542

Open
panicbit opened this issue Dec 24, 2020 · 2 comments
Open

MessageBuffer allows unsafe out-of-bounds reads and writes #542

panicbit opened this issue Dec 24, 2020 · 2 comments
Assignees

Comments

@panicbit
Copy link

panicbit commented Dec 24, 2020

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 panicbit changed the title MessageBuffer allows unsafe out-of-bounds reads and writes MessageBuffer allows out-of-bounds reads and writes Dec 24, 2020
@panicbit panicbit changed the title MessageBuffer allows out-of-bounds reads and writes MessageBuffer allows unsafe out-of-bounds reads and writes Dec 27, 2020
@komamitsu
Copy link
Member

@xerial Can you take a look at this?

xerial added a commit to xerial/msgpack-java that referenced this issue Feb 14, 2021
@xerial
Copy link
Member

xerial commented Feb 14, 2021

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants