From 2110f86e515b7c44d16bb89b6943e799123d84f6 Mon Sep 17 00:00:00 2001 From: Michael Tautschnig Date: Sun, 19 Jul 2026 19:58:01 +0000 Subject: [PATCH 1/3] Fix BigInt::div computing quotient from divisor instead of dividend In the single-digit-divisor branch of the combined quotient/remainder method BigInt::div, the quotient was initialized from the divisor (q = y) instead of the dividend (q = x). As digit_div transforms the dividend in place into the quotient, any division of a dividend exceeding 64 bits by a divisor fitting a single 32-bit digit returned q = 1, r = 0 (the divisor divided by itself). The bug dates back to the initial import of the big-int library. It went unnoticed because the only in-tree caller of BigInt::div is modinv, which itself has no callers, and the number.tst data driving the existing unit test never combines a dividend of more than 64 bits with a single-digit divisor. Add unit tests covering exactly that combination, for positive and negative dividends and divisors as well as a zero remainder, and also covering the long-division branch of BigInt::div, which was previously never exercised directly either. Co-authored-by: Kiro --- src/big-int/bigint.cc | 2 +- unit/big-int/big-int.cpp | 53 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/big-int/bigint.cc b/src/big-int/bigint.cc index 978c63e2ad0..442d41d055a 100644 --- a/src/big-int/bigint.cc +++ b/src/big-int/bigint.cc @@ -1076,7 +1076,7 @@ BigInt::div (BigInt const &x, BigInt const &y, BigInt &q, BigInt &r) else if (y.length == 1) { // This digit_div() transforms the dividend into the quotient. - q = y; + q = x; r.digit[0] = digit_div (q.digit, q.length, y.digit[0]); r.length = r.digit[0] ? 1 : 0; } diff --git a/unit/big-int/big-int.cpp b/unit/big-int/big-int.cpp index 678318bf238..f1b50354140 100644 --- a/unit/big-int/big-int.cpp +++ b/unit/big-int/big-int.cpp @@ -75,6 +75,59 @@ TEST_CASE("arbitrary precision integers", "[core][big-int][bigint]") REQUIRE(to_string(i) == "1"); } + // ===================================================================== + // Combined division/remainder. + // ===================================================================== + // BigInt::div used to compute the quotient from the divisor instead of + // the dividend when the dividend exceeded 64 bits and the divisor fit a + // single digit (32 bits). + SECTION("combined division/remainder with single-digit divisor") + { + const BigInt dividend = pow(BigInt{2}, 89) - 1; + const BigInt divisor{97}; + BigInt q, r; + + BigInt::div(dividend, divisor, q, r); + REQUIRE(to_string(q) == "6381134223120516880923320"); + REQUIRE(to_string(r) == "71"); + REQUIRE(q * divisor + r == dividend); + + // Negative dividend: truncated division, remainder takes the + // dividend's sign. + BigInt::div(-dividend, divisor, q, r); + REQUIRE(to_string(q) == "-6381134223120516880923320"); + REQUIRE(to_string(r) == "-71"); + REQUIRE(q * divisor + r == -dividend); + + // Negative divisor: the quotient flips sign, the remainder keeps + // the dividend's sign. + BigInt::div(dividend, -divisor, q, r); + REQUIRE(to_string(q) == "-6381134223120516880923320"); + REQUIRE(to_string(r) == "71"); + REQUIRE(q * -divisor + r == dividend); + + // Both negative: the quotient is positive, the remainder keeps the + // dividend's sign. + BigInt::div(-dividend, -divisor, q, r); + REQUIRE(to_string(q) == "6381134223120516880923320"); + REQUIRE(to_string(r) == "-71"); + REQUIRE(q * -divisor + r == -dividend); + + // Zero remainder exercises the r.length = 0 path. + BigInt::div(dividend * divisor, divisor, q, r); + REQUIRE(q == dividend); + REQUIRE(r.is_zero()); + + // A divisor of more than one digit exercises the long-division + // branch, which is otherwise only tested via operator/= and + // operator%= that have separate copies of that code. + const BigInt divisor2 = pow(BigInt{2}, 41) + 3; + BigInt::div(dividend, divisor2, q, r); + REQUIRE(to_string(q) == "281474976710272"); + REQUIRE(to_string(r) == "1151"); + REQUIRE(q * divisor2 + r == dividend); + } + // ===================================================================== // Test cases from the clisp test suite in number.tst. // ===================================================================== From dec5642521dc97ceaac417e325650a251001f12f Mon Sep 17 00:00:00 2001 From: Michael Tautschnig Date: Wed, 22 Jul 2026 18:51:36 +0000 Subject: [PATCH 2/3] Fix out-of-bounds read in BigInt::div when growing output operands The general-division branch of BigInt::div set the output operands' lengths before resizing them. BigInt::resize copies `length` digits from the previous buffer into the newly allocated one, so whenever the quotient (or remainder) needed more digits than the output operand's current buffer held, resize read past the end of that buffer. The copied garbage was subsequently overwritten, leaving results correct, but the read is undefined behavior and is flagged by address sanitizer. Resize first and set the length afterwards, so that resize only copies the digits that are actually valid. The existing tests mask this because the number.tst-driven loop reuses output operands whose buffers have grown large in earlier iterations; the next commit, which makes div return freshly constructed values, would expose the over-read in every address sanitizer run. Co-authored-by: Kiro --- src/big-int/bigint.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/big-int/bigint.cc b/src/big-int/bigint.cc index 442d41d055a..eb84b78dc19 100644 --- a/src/big-int/bigint.cc +++ b/src/big-int/bigint.cc @@ -1105,9 +1105,11 @@ BigInt::div (BigInt const &x, BigInt const &y, BigInt &q, BigInt &r) if (a[al-1] >= b[bl-1]) a[al++] = 0; - // Prepare q for receiving the quotient. + // Prepare q for receiving the quotient. Set the length only + // after resizing: resize copies `length` digits from the old + // buffer, which may be shorter than the new length. + q.resize(al - bl); q.length = al - bl; - q.resize (q.length); // Divide. digit_div (a, b, bl, q.digit, q.length); @@ -1117,8 +1119,8 @@ BigInt::div (BigInt const &x, BigInt const &y, BigInt &q, BigInt &r) if (scale != 1) digit_div (a, al, scale); if (al && a[al - 1] == 0) --al; + r.resize(al); r.length = al; - r.resize (r.length); memcpy (r.digit, a, al * sizeof (onedig_t)); } q.adjust(); From 5bfb655f94a9ab6efae1df16f6b67c38e42cecb1 Mon Sep 17 00:00:00 2001 From: Michael Tautschnig Date: Wed, 22 Jul 2026 18:53:26 +0000 Subject: [PATCH 3/3] Make BigInt::div return its results, precluding aliasing BigInt::div wrote into caller-provided output arguments before it was done reading the inputs: div(x, y, y, r) clobbered the divisor before its first digit was read and silently computed garbage, and passing the same object for quotient and remainder cannot produce a valid result at all. Rather than documenting the restriction (which leaves the breakage silent) or detecting aliased arguments at runtime, change the interface so that the aliasing cannot be expressed in the first place: div now returns a divisiont struct holding freshly constructed quotient and remainder values. Named return value optimization applies, so the non-aliased case does not pay for a copy. On the division-by-zero error path the returned values are now deterministically zero, where previously the output arguments were left untouched after printing the error. Adjust the only in-tree caller, modinv, and the unit tests, which as a side effect no longer need pre-declared output variables. Co-authored-by: Kiro --- src/big-int/bigint-func.cc | 10 +++---- src/big-int/bigint.cc | 14 +++++---- src/big-int/bigint.hh | 17 +++++++++-- unit/big-int/big-int.cpp | 61 +++++++++++++++++++------------------- 4 files changed, 59 insertions(+), 43 deletions(-) diff --git a/src/big-int/bigint-func.cc b/src/big-int/bigint-func.cc index 6ee5210a5c6..b71b6c9c328 100644 --- a/src/big-int/bigint-func.cc +++ b/src/big-int/bigint-func.cc @@ -94,16 +94,16 @@ modinv (const BigInt &a, const BigInt &m) { BigInt j (1), i (0); BigInt b (m), c (a); - BigInt x, y; + BigInt y; while (!c.is_zero()) { - BigInt::div (b, c, x, y); + BigInt::divisiont qr = BigInt::div(b, c); b = c; - c = y; + c = qr.remainder; y = j; - // j = i - j * x; trading clarity for efficiency. - j *= x; + // j = i - j * qr.quotient; trading clarity for efficiency. + j *= qr.quotient; j -= i; j.negate(); diff --git a/src/big-int/bigint.cc b/src/big-int/bigint.cc index eb84b78dc19..2aa28e47f83 100644 --- a/src/big-int/bigint.cc +++ b/src/big-int/bigint.cc @@ -1033,9 +1033,12 @@ BigInt::operator*= (BigInt const &y) // Division method returning both quotient and remainder. -void -BigInt::div (BigInt const &x, BigInt const &y, BigInt &q, BigInt &r) +BigInt::divisiont BigInt::div(BigInt const &x, BigInt const &y) { + divisiont result; + BigInt &q = result.quotient; + BigInt &r = result.remainder; + // Eliminate some trivial cases. int cmp = x.ucompare (y); if (cmp < 0) @@ -1044,7 +1047,7 @@ BigInt::div (BigInt const &x, BigInt const &y, BigInt &q, BigInt &r) r = x; q.length = 0; q.positive = true; - return; + return result; } if (cmp == 0) { @@ -1055,13 +1058,13 @@ BigInt::div (BigInt const &x, BigInt const &y, BigInt &q, BigInt &r) q.digit[0] = 1; if (!y.positive) q.negate(); - return; + return result; } if (y.length == 0) { zero: error ("Division by zero."); - return; + return result; } if (x.is_ulong()) { @@ -1126,6 +1129,7 @@ BigInt::div (BigInt const &x, BigInt const &y, BigInt &q, BigInt &r) q.adjust(); q.positive = q.length == 0 || x.positive == y.positive; r.positive = r.length == 0 || x.positive; + return result; } BigInt & diff --git a/src/big-int/bigint.hh b/src/big-int/bigint.hh index 4abaca6abaa..57c3aaef583 100644 --- a/src/big-int/bigint.hh +++ b/src/big-int/bigint.hh @@ -270,8 +270,16 @@ public: BigInt &operator++ () { return operator+=(1); } // preincrement BigInt &operator-- () { return operator-=(1); } // predecrement - static void div (BigInt const &, BigInt const &, - BigInt ", BigInt &rem) _fasta; + // Result of the combined quotient/remainder computation. Returning + // fresh objects (rather than filling in caller-provided output + // arguments) makes aliasing between inputs and outputs impossible by + // construction. The struct is defined after this class. + struct divisiont; + + // Combined quotient/remainder computation: quotient = x / y, + // remainder = x % y (truncated division, the remainder takes the + // sign of x). + static divisiont div(BigInt const &, BigInt const &) _fasta; // Returns the largest x such that 2^x <= abs() or 0 if input is 0 // Not part of original BigInt. @@ -290,6 +298,11 @@ public: } }; +struct BigInt::divisiont +{ + BigInt quotient; + BigInt remainder; +}; // Functions on BigInt. Implementations in bigint-func.cc. diff --git a/unit/big-int/big-int.cpp b/unit/big-int/big-int.cpp index f1b50354140..b6528b1d4a8 100644 --- a/unit/big-int/big-int.cpp +++ b/unit/big-int/big-int.cpp @@ -85,47 +85,46 @@ TEST_CASE("arbitrary precision integers", "[core][big-int][bigint]") { const BigInt dividend = pow(BigInt{2}, 89) - 1; const BigInt divisor{97}; - BigInt q, r; - BigInt::div(dividend, divisor, q, r); - REQUIRE(to_string(q) == "6381134223120516880923320"); - REQUIRE(to_string(r) == "71"); - REQUIRE(q * divisor + r == dividend); + BigInt::divisiont qr = BigInt::div(dividend, divisor); + REQUIRE(to_string(qr.quotient) == "6381134223120516880923320"); + REQUIRE(to_string(qr.remainder) == "71"); + REQUIRE(qr.quotient * divisor + qr.remainder == dividend); // Negative dividend: truncated division, remainder takes the // dividend's sign. - BigInt::div(-dividend, divisor, q, r); - REQUIRE(to_string(q) == "-6381134223120516880923320"); - REQUIRE(to_string(r) == "-71"); - REQUIRE(q * divisor + r == -dividend); + qr = BigInt::div(-dividend, divisor); + REQUIRE(to_string(qr.quotient) == "-6381134223120516880923320"); + REQUIRE(to_string(qr.remainder) == "-71"); + REQUIRE(qr.quotient * divisor + qr.remainder == -dividend); // Negative divisor: the quotient flips sign, the remainder keeps // the dividend's sign. - BigInt::div(dividend, -divisor, q, r); - REQUIRE(to_string(q) == "-6381134223120516880923320"); - REQUIRE(to_string(r) == "71"); - REQUIRE(q * -divisor + r == dividend); + qr = BigInt::div(dividend, -divisor); + REQUIRE(to_string(qr.quotient) == "-6381134223120516880923320"); + REQUIRE(to_string(qr.remainder) == "71"); + REQUIRE(qr.quotient * -divisor + qr.remainder == dividend); // Both negative: the quotient is positive, the remainder keeps the // dividend's sign. - BigInt::div(-dividend, -divisor, q, r); - REQUIRE(to_string(q) == "6381134223120516880923320"); - REQUIRE(to_string(r) == "-71"); - REQUIRE(q * -divisor + r == -dividend); + qr = BigInt::div(-dividend, -divisor); + REQUIRE(to_string(qr.quotient) == "6381134223120516880923320"); + REQUIRE(to_string(qr.remainder) == "-71"); + REQUIRE(qr.quotient * -divisor + qr.remainder == -dividend); - // Zero remainder exercises the r.length = 0 path. - BigInt::div(dividend * divisor, divisor, q, r); - REQUIRE(q == dividend); - REQUIRE(r.is_zero()); + // Zero remainder exercises the remainder.length = 0 path. + qr = BigInt::div(dividend * divisor, divisor); + REQUIRE(qr.quotient == dividend); + REQUIRE(qr.remainder.is_zero()); // A divisor of more than one digit exercises the long-division // branch, which is otherwise only tested via operator/= and // operator%= that have separate copies of that code. const BigInt divisor2 = pow(BigInt{2}, 41) + 3; - BigInt::div(dividend, divisor2, q, r); - REQUIRE(to_string(q) == "281474976710272"); - REQUIRE(to_string(r) == "1151"); - REQUIRE(q * divisor2 + r == dividend); + qr = BigInt::div(dividend, divisor2); + REQUIRE(to_string(qr.quotient) == "281474976710272"); + REQUIRE(to_string(qr.remainder) == "1151"); + REQUIRE(qr.quotient * divisor2 + qr.remainder == dividend); } // ===================================================================== @@ -227,15 +226,15 @@ TEST_CASE("arbitrary precision integers", "[core][big-int][bigint]") REQUIRE(m == em); // Also try the method returning both. - BigInt::div(a, b, r, m); + BigInt::divisiont qr = BigInt::div(a, b); // Again, transform to floored divide. - if(!m.is_zero() && a.is_positive() != b.is_positive()) + if(!qr.remainder.is_zero() && a.is_positive() != b.is_positive()) { - r -= 1; - m += b; + qr.quotient -= 1; + qr.remainder += b; } - REQUIRE(r == er); - REQUIRE(m == em); + REQUIRE(qr.quotient == er); + REQUIRE(qr.remainder == em); } } }