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

[BUG] Inconsistent behaviour of Lua msgpack between platforms #11862

Open
nicknotfun opened this issue Feb 28, 2023 · 6 comments
Open

[BUG] Inconsistent behaviour of Lua msgpack between platforms #11862

nicknotfun opened this issue Feb 28, 2023 · 6 comments
Labels
state:help-wanted No member is currently implementing this change

Comments

@nicknotfun
Copy link

nicknotfun commented Feb 28, 2023

Describe the bug

The results of Lua cmsgpack are dependent on the host platform.

To reproduce

eval "return cmsgpack.pack(cmsgpack.unpack('\\211\\127\\255\\255\\255\\255\\255\\255\\255'))" 0

This binary string is a msgpack encoded 64bit signed integer - specifically 9223372036854775807
The first byte is 0xd3 which denotes this format.

Running this command on Redis 7.0.8 / OSX (Apple Silicon M2 - OSX 13.1):

"\xcf\x7f\xff\xff\xff\xff\xff\xff\xff"

This is the 64bit unsigned encoding of the same number (so 'fine')

Running this command on Redis 7.0.8 / Linux (Ubuntu 22.04.2 LTS):

"\xca_\x00\x00\x00"

This is a float32 encoding of roughly the right value.

Expected behavior

Actually unsure, what is the correct behaviour, as Lua 5.1 doesn't support 64-bit integers I am somewhat surprised by the OSX response being lossless - but at the same time i'm surprised at the float32-float64 conversion that occurred in the Linux variant.

My expected result is more that both OSen should have identical output.

Additional information

I personally favour the route of an accurate-as-possible int64/float64 result.

@sundb
Copy link
Collaborator

sundb commented Mar 1, 2023

Because the precision of double is not consistent across platforms, IS_INT64_EQUIVALENT is inconsistent in determining whether 9223372036854775807 is int64.
so cmsgpack.pack will pack 9223372036854775807 as int64(\xcf\x7f\xff\xff\xff\xff\xff\xff\xff) or double(\xca_\x00\x00\x0)

simple test

#include <stdio.h>
#include <stdint.h>

int main() {
    double d = 9223372036854775807;
    printf("%d\n", (int64_t)d == d);
}

# output
ubuntu: 0
mac: 1

@nicknotfun
Copy link
Author

Suspected that would be the case, it does suggest a better approach however might be available? I'd have expected 'same in same out' for Redis x-platform.

@sundb
Copy link
Collaborator

sundb commented Mar 2, 2023

@nicknotfun i don't know how to do that.
just like the following INCRBYFLOAT command example that also shows different results on different platforms.

> INCRBYFLOAT key 9223372036854775807

# mac
"9223372036854775808"

# ubuntu
"9223372036854775807"

The ways I can think of are:

  1. treat 9223372036854775807 as a string.
  2. upgrade the version of lua to 5.3

@nicknotfun
Copy link
Author

nicknotfun commented Mar 2, 2023

In the case of msgpack it arguably should just only ever encode as the most precise it can, e.g. something like:

<=32bit, integer = i32
<=53bit, integer = i64
f32 if lossless, otherwise f64

Nothing guarantees the precise representation of the 53+bit numbers ; and without this it means you cannot reliably communicate with Redis-Lua. The incrbyfloat example I think is distinct as it's solely on Redis to decide; but msgpack is a data interchange format, and round-tripping is an important property for it to have for communication

@sundb
Copy link
Collaborator

sundb commented Mar 3, 2023

@nicknotfun In lua(5.3) there is support for storing number in a similar way to what you said, when the number can be stored to int64, it will be `lua_integer(int64_t), otherwise lua_Number(double).
but in lua(<5.3) all the numbers are always stored as lua_Number, so precision problem is unavoidable.

@nicknotfun
Copy link
Author

I agree lua 5.3 is better, but under 5.1 I think it should prefer consistency.

@sundb sundb added the state:help-wanted No member is currently implementing this change label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:help-wanted No member is currently implementing this change
Projects
None yet
Development

No branches or pull requests

2 participants