Skip to content

Default max_buffer_size causes silent failures when streaming #503

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

Closed
hauntsaninja opened this issue Apr 19, 2022 · 3 comments · Fixed by #506
Closed

Default max_buffer_size causes silent failures when streaming #503

hauntsaninja opened this issue Apr 19, 2022 · 3 comments · Fixed by #506

Comments

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Apr 19, 2022

Thanks for a useful library!

Consider the following repro:

import os
import msgpack
import tempfile

original_values = [
    (b"x" * 1024) * (1024 * 40 * i)
    for i in range(1, 5)
]

with tempfile.TemporaryDirectory() as tmpdir:
    fn = os.path.join(tmpdir, "test.msgpack")
    with open(fn, "wb") as f:
        for v in original_values:
            msgpack.pack(v, f)
    
    with open(fn, "rb") as f:
        unpack = msgpack.Unpacker(f, max_buffer_size=0)
        values = list(unpack)
    assert values == original_values

    with open(fn, "rb") as f:
        unpack = msgpack.Unpacker(f)
        values = list(unpack)
    # failed to unpack some of the stream, but we wouldn't know if we didn't have original_values to check against
    assert values == original_values
@hauntsaninja
Copy link
Contributor Author

This was changed in #319 to avoid DoS. That's fine, but ideally the above code would fail loudly, rather than silently.

Maybe I'm using the library wrong, but iterating over a msgpack.Unpacker is recommended in the README without any mention of this: https://github.com/msgpack/msgpack-python#streaming-unpacking

@methane
Copy link
Member

methane commented Apr 26, 2022

Thank you for reporting. I agree that Unpacker should raise an error when a message is larger than max_buffer_size.

hauntsaninja pushed a commit to hauntsaninja/msgpack-python that referenced this issue Apr 26, 2022
Fixes msgpack#503

Raising ValueError matches the behaviour of the pure Python version.
I'm not sure that this is the best place to raise this message; it might
make more sense in `append_buffer`. It's also conceivable that we want
to raise BufferFull instead of ValueError or something.
@hauntsaninja
Copy link
Contributor Author

Thanks! I opened a PR with a potential fix, although not sure it's correct #504 :-)

hauntsaninja pushed a commit to hauntsaninja/msgpack-python that referenced this issue Apr 26, 2022
Fixes msgpack#503

Raising ValueError matches the behaviour of the pure Python version.
I'm not sure that this is the best place to raise this message; it might
make more sense when reading headers or in `append_buffer` or
something. It's also possible that we want to raise BufferFull instead
of ValueError.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants