search

Bakkes / CPPRP

Fast C++ Rocket League replay parser
Mozilla Public License 2.0
32 stars 9 forks source link

CPPRPJSON: Numbers may exceed signed long (edge case) #4

Closed aidant19 closed 3 years ago

aidant19 commented 3 years ago

The case in which I found this to occur is in a replay where a unique identifier from a player came from the NNX platform. It seems the identifier is within the range of an unsigned long (2^64), but out of range for a signed long. This presents a potential interoperability issue in languages that do not support unsigned integers (I personally came across this in Java).

A potential fix would be to treat such IDs as a string.

Disclaimer: this is technically not out of spec for JSON, but I thought I'd document it anyway. If you choose to implement this fix my JSON parser will be happy, but since this is technically not a problem for C++ no worries if you would prefer to avoid potential complications like this. As well, this would technically not occur if the parser did not attempt to strictly convert data types.

Bakkes commented 3 years ago

Do you have a replay and JSON output where this is the case? The JSON output of the parser is used to process many replays a day in some node apps, where the max value of a number is 2^53 I believe and I haven't heard about any issues there (but maybe those apps discard non-steam and epic players).

https://github.com/Bakkes/CPPRP/commit/7a757b13334482b3be309bcd06cc315115383edd Looks like these IDs are supposed to be serialized to string already anyway. If you've got a replay where it serializes to a number that's too big please upload it here!

aidant19 commented 3 years ago

Here is the replay file and resulting JSON file that was produced: example.zip

Addendum: The value in question is: 14661134270990616052

Bakkes commented 3 years ago

Oh it's still serializing as a number in the header, interesting. Thanks for uploading the replay. I've gotta check to see if it breaks any production code that might rely on this but I doubt it. Will get back to this.

Bakkes commented 3 years ago

Alright, all unsigned long longs in the header will now be serialized as a string! You're probably going to run into issues when implementing logic for epic players as their onlineID is always 0 and you'll have to read the UniqueNetId data from the body instead of the header though.

I won't merge these changes to master for a while, but you can check-out and compile cb39768 or use the attached binaries which include this change. cpprp_binaries.zip

Thanks for the suggestion, let me know if there are any other issues!

aidant19 commented 3 years ago

Perfect! Thank you.