Skip to content

Conversation

@lcallarec
Copy link

Hey,

This PR fixes an integer overflow issue in both AssemblyScript & Typescript implementations.

In short : on an unsigned integer, when overflow occurs, the value won't be read as a negative integer.

Please note that in the current implementation, the user could create an Histogram with a signed internal bucket of type IntXArray ; an issue will also occurs and this PR doesn't fix that case. :)

Note also that I didn't find a way to test the assembly version in assembly folder : the tested method throws, and it seems that try / catch aren't supported yet

@alexvictoor
Copy link
Member

Thanks a lot for the PR!
I guess there might be an issue with "record value with count" methods as well

@lcallarec
Copy link
Author

lcallarec commented Nov 17, 2020

You're right : overflows may produce wrong counts when :

  • addition and subtraction of histograms
  • decoding an histogram as a smaller sized one.

I'll take a look.

@lcallarec lcallarec force-pushed the fix-integer-overflows branch 2 times, most recently from ea9c823 to dbe20fe Compare November 19, 2020 11:41
@lcallarec lcallarec force-pushed the fix-integer-overflows branch from dbe20fe to 74ebc8b Compare November 19, 2020 12:25
@lcallarec
Copy link
Author

I guess all remaining overflow issues have been fixed with last commits.

@lcallarec
Copy link
Author

Sorry @MaxGraey, I don't know what happend, looks like I did something that deleted your commit proposal (or there was a github issue) with my comment... I was saying that the bitwise shift operation won't work as is. maxBucketSize is typed as number to avoid TS2365: Operator '>' cannot be applied to types 'f64' and 'i64' compiler errors.

So I guess that we should writethis.maxBucketSize = ((1 << (<i8>sizeof<U>() * 8)) - 1) as number;. What do you think ?

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 this pull request may close these issues.

2 participants