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

Strings of integers over 2^63-1 gets encoded as float64 instead of uint64 #487

Closed
aesy opened this issue Oct 15, 2018 · 1 comment
Closed

Comments

@aesy
Copy link

aesy commented Oct 15, 2018

The problem lies in the following code:

@Override
public void writeNumber(String encodedValue)
throws IOException, JsonGenerationException, UnsupportedOperationException
{
// There is a room to improve this API's performance while the implementation is robust.
// If users can use other MessagePackGenerator#writeNumber APIs that accept
// proper numeric types not String, it's better to use the other APIs instead.
try {
long l = Long.parseLong(encodedValue);
addValueToStackTop(l);
return;
}
catch (NumberFormatException e) {
}
try {
double d = Double.parseDouble(encodedValue);
addValueToStackTop(d);
return;
}
catch (NumberFormatException e) {
}
try {
BigInteger bi = new BigInteger(encodedValue);
addValueToStackTop(bi);
return;
}
catch (NumberFormatException e) {
}
try {
BigDecimal bc = new BigDecimal(encodedValue);
addValueToStackTop(bc);
return;
}
catch (NumberFormatException e) {
}
throw new NumberFormatException(encodedValue);
}

I will provide an example. Take the following input: "9223372036854775808" (2^63)
Since Javas Long type max value is 2^63-1, the first try will fail, continuing to the next try. Double will succeed resulting in the input being encoded as a float64. As a user of the library I would've assumed that the input got encoded as a uint64.

I'm not sure, but it may also be a problem that integers can silently lose precision. I think an IllegalArgumentException would've been preferred in that case, unless there are decimals in the number (even if just a trailing zero).

Double#parseDouble will succeed to parse all integers, although integers past Double.MAX_VALUE will yield Infinity, so the try to parse BigInteger will either never be reached or never succeed for any input.

Unless I'm mistaken, these issues could be solved by swapping the ordering of the parse tries, placing BigInteger before Double, and in that case I don't think it's even necessary to try parse Long and Double rather than just BigInteger and BigDecimal. Maybe it's less performant?

I realize that the other writeNumber methods are preferred over this one. I'm using a library that has a dependency on this library (dd-trace-java) where the same assumption that a string of a 64-bit integer would be encoded as a uint64 was made.

I will make a pull request shortly.

@aesy
Copy link
Author

aesy commented Apr 3, 2024

Closing because of inactivity

@aesy aesy closed this as completed Apr 3, 2024
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

Successfully merging a pull request may close this issue.

1 participant