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

Arithmetic overflow in BigDecimal #10502

Closed
SlayerShadow opened this issue Mar 12, 2021 · 6 comments · Fixed by #10628
Closed

Arithmetic overflow in BigDecimal #10502

SlayerShadow opened this issue Mar 12, 2021 · 6 comments · Fixed by #10628
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:numeric

Comments

@SlayerShadow
Copy link

I got these values when represented Bresenham's algorithm. Reduced as possible.

require "big"

a = BigDecimal.new "-0.0000000000000005"
b = BigDecimal.new "0.9999999999999979291832202937938"

p a.div( b )

Provides an error:

Unhandled exception: Arithmetic overflow (OverflowError)
  from /usr/lib/crystal/core/big/big_decimal.cr:210:20 in 'div'

Carc (Crystal 0.36.1).

Tested against modern compiler too:

# crystal -v
Crystal 1.0.0-dev [2588dc5] (2021-03-10)

LLVM: 11.1.0
Default target: x86_64-unknown-linux-musl
@SlayerShadow SlayerShadow added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Mar 12, 2021
@bcardiff
Copy link
Member

The overflow comes from the fact that at https://github.com/crystal-lang/crystal/blob/master/src/big/big_decimal.cr#L210 , scale = @scale - other.scale is an UInt64 operation with:

@scale              # => 16
other.scale         # => 31

It seems it's a bug that was always present in BigDecimal implementation.

@bcardiff bcardiff added good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. topic:stdlib:numeric labels Mar 12, 2021
@kellydanma
Copy link
Contributor

I was looking into this bug because I also encountered the unhandled exception. I think the underlying problem is beyond the div method; Rust uses a signed integer for the equivalent of @scale, not an unsigned one.

Before anyone picks up this issue, why did we choose UInt64 for the instance variable type in this class?

@straight-shoota
Copy link
Member

straight-shoota commented Apr 12, 2021

The data type was already UInt64 in the first commit that brought the BigDecimal implementation to Crystal: bc54aa0

It even references https://github.com/akubera/bigdecimal-rs as source of inspiration, but for some reason the scale type was changed to unsigned. Probably just a mistake and we should change it to Int64.

@straight-shoota
Copy link
Member

Otoh, scale itself can't really be negative, so we could just fix #div implementation.

@kellydanma
Copy link
Contributor

👆 Fixing #div makes more sense to me, thanks!

@kellydanma
Copy link
Contributor

I'm a newbie 👋
I started working on this issue, feel free to comment even if the PR is in draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:numeric
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants