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

Buffer over/underrun using UBJson? #1288

Closed
abrownsword opened this issue Oct 9, 2018 · 11 comments
Closed

Buffer over/underrun using UBJson? #1288

abrownsword opened this issue Oct 9, 2018 · 11 comments
Assignees
Labels
aspect: binary formats BSON, CBOR, MessagePack, UBJSON documentation release item: ⚡ improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@abrownsword
Copy link

I'm using 3.2.0 and have stumbled upon what seems like a buffer over or underrun error.

std::vector<uint8_t> output;
json object = SerializeMyData(data);
json::to_ubjson(object, output);

This operation appears to work, however afterward there is a problem that appears to be caused by memory elsewhere being corrupted. If I replace to_ubjson with to_cbor, to_msgpack, or dump the problem does not occur.

The code in question is buried deep in other code and the data difficult to extract, so creating a repro case will take some effort which I can't spare at the moment. Plus I was just trying to_ubjson out of curiosity to compare sizes, so this isn't a blocking issue for me. A quick search doesn't turn up an existing issue. I submit this issue in case it is enough information for the author to zero in on a potential problem, or if someone else has seen something similar.

@nlohmann
Copy link
Owner

nlohmann commented Oct 9, 2018

Thanks for reporting, but without any further information on the input, the nature of the error, example code, etc., it is difficult to act upon the issue. Any hint is greatly appreciated!

@nlohmann nlohmann added state: needs more info the author of the issue needs to provide more details aspect: binary formats BSON, CBOR, MessagePack, UBJSON labels Oct 9, 2018
@abrownsword
Copy link
Author

Yes, I understand and will see if I can set some time aside to dig deeper and create a repro case.

@abrownsword
Copy link
Author

abrownsword commented Oct 12, 2018

Have more information. After jostling the code around a little, my debugger started to show me that to_ubjson is actually throwing an exception (why it wasn't showing that all along is concerning and I haven't pursued it yet, but probably not related to this library).

[json.exception.out_of_range.407] number overflow serializing 18446744073709551516

Adding a try/catch block suppresses the error and the memory corruption originally described goes away, so I suspect that is symptomatic of the exception handling problem rather than the json library.

That value is, of course, 0xffffffffffffff9c and therefore only representable (among the primitive types) as uint64_t. Is this not representable in UBJSON, or is there an oversight near line 766 of binary_writer.cpp?

@nlohmann
Copy link
Owner

If I interpret http://ubjson.org/type-reference/value-types/#numeric correctly, then UBJSON only supports signed 64 bit integers, so 18446744073709551516 is not representable.

I can reproduce your issue with

#include "json.hpp"

using nlohmann::json;

int main()
{
    json j = 18446744073709551516;
    auto x = json::to_ubjson(j);
}

Throwing

libc++abi.dylib: terminating with uncaught exception of type nlohmann::detail::out_of_range: [json.exception.out_of_range.407] number overflow serializing 18446744073709551516

So I would say the library works as expected, right?

@abrownsword
Copy link
Author

Given that UBJSON is, ummm, "limited" (broken if I'm less charitable)... then yes, your library is working correctly.

@nlohmann
Copy link
Owner

UBJSON still has a "high-precision number type" which is basically the string representation of the number:

The high-precision number type is an ultra-portable mechanism by which arbitrarily large (or precise) numbers, greater than 64-bit in size, are encoded as a UTF-8 string and passed between systems that support them. This allows high-precision number values to degrade gracefully on systems that do not have a built-in type to support numeric values larger than 64-bit. Please refer to the Best Practices page for techniques on working around the lack of larger-than-64-bit numeric types on certain platforms if you need them.

The library does not support this. Do you think this would be helpful?

@abrownsword
Copy link
Author

Not to me -- I won't use UBJSON now that I know it doesn't support unsigned 64-bit. If I were forced to then, yes, it would be useful as I tend to have a lot of max or near-max uint64_t values.

@nlohmann
Copy link
Owner

I would go with CBOR if possible.

@abrownsword
Copy link
Author

One thing that would be nice (as a user of the library) would be if somehow the message from the exception could state that this is a limitation of the chosen format. Otherwise it isn't obvious why the value is out of range.

@nlohmann
Copy link
Owner

What do you mean? Something like

[json.exception.out_of_range.407] number overflow serializing 18446744073709551516; number cannot be represented by UBJSON

or what would you propose?

@nlohmann nlohmann added documentation and removed state: needs more info the author of the issue needs to provide more details labels Oct 16, 2018
@abrownsword
Copy link
Author

Yeah, that would be helpful. It points out to the user that its not a bug per se, but a format limitation.

nlohmann added a commit that referenced this issue Oct 17, 2018
This commit is the equivalent of #1282 for CBOR, MessagePack, and UBJSON.
@nlohmann nlohmann self-assigned this Oct 17, 2018
@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation release item: ⚡ improvement labels Oct 17, 2018
@nlohmann nlohmann added this to the Release 3.3.1 milestone Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aspect: binary formats BSON, CBOR, MessagePack, UBJSON documentation release item: ⚡ improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

2 participants