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

Fix issue 20493 - incorrect result of BigInt * BigInt #7349

Merged
merged 1 commit into from
Jan 11, 2020

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Jan 11, 2020

The problem comes down to a buffer overflow.
How could a buffer overflow happen in @safe code?
Well, it's basically a logical buffer overflow, while technically no out of bounds memory is accessed.

Two buffers are created in one allocation of size a + b (presumably as an optimization).
buffer[a..$] is given to partial, while scratchbuf remains the whole buffer[0..$] instead of buffer[0..a].
That means that if more than a elements are written to scratchbuf, it messes up partial, which is what happens in the bug report.

The buffer size for Karatsuba multiplication of x * y is calculated as 2 * x.length (x and y are a BigDigit[] which is a uint[]).
Most calls to mulKaratsuba claim 2 * half(x.length) of scratchbuf and pass the rest to a recursive call of half the instance size. I think the underlying assumption is that the geometric series 1 + 0.5 + 0.25 ... <= 2 so the bound is sufficient.

It fails to account for two things however:

  • The problem size is integer, and when it is halved, it is rounded up. So when x and y have length 641, the next problem size is 321, then 161, 81 etc. Every time a little more than half of the available space is claimed.
  • When the multiplication of the recursive case is unbalanced (i.e. x.length is more than y.length * sqrt(2)), another half of the buffer is claimed.

So why wasn't this caught earlier? Well, there is a KARATSUBALIMIT currently set to 10, and a BigDigit[] with length < KARATSUBALIMIT gets a small size optimization so no Karatsuba multiplication is done. This limit gave some constant leeway to the buffer. When numbers get sufficiently large, this asymptotic growth of the required buffer size catches up with the constant leeway of the actual buffer size with lower asymptotic growth however.

The smallest case of this bug with the current limit is when (x.length, y.length) = (116, 99), so this bug only manifests once numbers reach a size of 2^(32*116) ~= 10^1117 which is probably why it got undetected for so long.

Finding the correct minimum buffer size is hard, since it depends on KARATSUBALIMIT (for base cases), log2(x.length) (for rounding errors), and how often the recursive case is unbalanced.
With some brute force testing I found that a little over factor 2 seems sufficient for at least 10 000 big digits even with a smaller KARATSUBALIMIT of 2, so I settled with 2.25 for now.

I also truncated the scratchbuff so that at least instead of silently giving an incorrect answer, it raises a RangeError on failure.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 11, 2020

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20493 normal Incorrect result of BigInt * BigInt

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7349"

@@ -2224,10 +2251,12 @@ bool inplaceSub(BigDigit[] result, const(BigDigit)[] x, const(BigDigit)[] y)

/* Determine how much space is required for the temporaries
* when performing a Karatsuba multiplication.
* TODO: determining a tight bound is non-trivial and depends on KARATSUBALIMIT, see:
* https://issues.dlang.org/show_bug.cgi?id=20493
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping me when you're happy with this TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to leave that for a future PR. The comment is there to explain that the formula is a bit arbitrary and not the result of rigorous mathematical analysis or benchmarking, but it should be good enough.

@dlang-bot dlang-bot merged commit d29ebfe into dlang:master Jan 11, 2020
@dkorpel dkorpel deleted the bigint-bug branch January 11, 2020 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants