Re: [PATCH 1/2 v2] libdecnumber: fixed multiple potential access-out-of bounds errors by moving range conditions before reads.
Hello—just pinging again about the following two patches I submitted. Each
fixes small access-out-of-bounds errors in libdecnumber.
https://gcc.gnu.org/pipermail/gcc-patches/2024-February/644840.html
https://gcc.gnu.org/pipermail/gcc-patches/2024-February/644910.html
Also, the documentation indicated that I should mention that I do not have
write access, since I'm a first-time contributor.
On Sat, Feb 3, 2024 at 2:31 PM Ian McCormack wrote:
> Multiple `for` loops across `libdecnumber` contain boolean expressions
> where memory is accessed prior to checking if the pointer is still within a
> valid range, which can lead to out-of-bounds reads.
>
> This patch moves the range conditions to appear before the memory accesses
> in each conjunction so that these expressions short-circuit instead of
> performing an invalid read.
>
> libdecnumber/ChangeLog
>* In each `for` loop and `if` statement, all boolean expressions of
> the form `*ptr == value && in_range(ptr)` have been changed to
> `in_range(ptr) && *ptr == value`.
>
> Bootstrapped on x86_64-pc-linux-gnu with no regressions.
> ---
> libdecnumber/decBasic.c | 20 ++--
> libdecnumber/decCommon.c | 2 +-
> libdecnumber/decNumber.c | 2 +-
> 3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/libdecnumber/decBasic.c b/libdecnumber/decBasic.c
> index 6319f66b25d..04833f2390d 100644
> --- a/libdecnumber/decBasic.c
> +++ b/libdecnumber/decBasic.c
> @@ -341,7 +341,7 @@ static decFloat * decDivide(decFloat *result, const
> decFloat *dfl,
> for (;;) { /* inner loop -- calculate quotient unit */
>/* strip leading zero units from acc (either there initially or */
>/* from subtraction below); this may strip all if exactly 0 */
> - for (; *msua==0 && msua>=lsua;) msua--;
> + for (; msua>=lsua && *msua==0;) msua--;
>accunits=(Int)(msua-lsua+1); /* [maybe 0] */
>/* subtraction is only necessary and possible if there are as */
>/* least as many units remaining in acc for this iteration as */
> @@ -515,7 +515,7 @@ static decFloat * decDivide(decFloat *result, const
> decFloat *dfl,
> /* (must also continue to original lsu for correct quotient length) */
> if (lsua>acc+DIVACCLEN-DIVOPLEN) continue;
> for (; msua>lsua && *msua==0;) msua--;
> -if (*msua==0 && msua==lsua) break;
> +if (msua==lsua && *msua==0) break;
> } /* outer loop */
>
>/* all of the original operand in acc has been covered at this point */
> @@ -1543,8 +1543,8 @@ decFloat * decFloatAdd(decFloat *result,
> umsd=acc+COFF+DECPMAX-1; /* so far, so zero */
> if (ulsd>umsd) { /* more to check */
> umsd++; /* to align after checked area */
> - for (; UBTOUI(umsd)==0 && umsd+3 - for (; *umsd==0 && umsd + for (; umsd+3 + for (; umsd }
> if (*umsd==0) {/* must be true zero (and diffsign) */
> num.sign=0; /* assume + */
> @@ -2087,10 +2087,10 @@ decFloat * decFloatFMA(decFloat *result, const
> decFloat *dfl,
>/* remove leading zeros on both operands; this will save time later */
>/* and make testing for zero trivial (tests are safe because acc */
>/* and coe are rounded up to uInts) */
> - for (; UBTOUI(hi->msd)==0 && hi->msd+3lsd;) hi->msd+=4;
> - for (; *hi->msd==0 && hi->msdlsd;) hi->msd++;
> - for (; UBTOUI(lo->msd)==0 && lo->msd+3lsd;) lo->msd+=4;
> - for (; *lo->msd==0 && lo->msdlsd;) lo->msd++;
> + for (; hi->msd+3lsd && UBTOUI(hi->msd)==0;) hi->msd+=4;
> + for (; hi->msdlsd && *hi->msd==0;) hi->msd++;
> + for (; lo->msd+3lsd && UBTOUI(lo->msd)==0;) lo->msd+=4;
> + for (; lo->msdlsd && *lo->msd==0;) lo->msd++;
>
>/* if hi is zero then result will be lo (which has the smaller */
>/* exponent), which also may need to be tested for zero for the */
> @@ -2252,8 +2252,8 @@ decFloat * decFloatFMA(decFloat *result, const
> decFloat *dfl,
>/* all done except for the special IEEE 754 exact-zero-result */
>/* rule (see above); while testing for zero, strip leading */
>/* zeros (which will save decFinalize doing it) */
> - for (; UBTOUI(lo->msd)==0 && lo->msd+3lsd;) lo->msd+=4;
> - for (; *lo->msd==0 && lo->msdlsd;) lo->msd++;
> + for (; lo->msd+3lsd && UBTOUI(lo->msd)==0;) lo->msd+=4;
> + for (; lo->msdlsd && *lo->msd==0;) lo
[PATCH 1/2 v2] libdecnumber: fixed multiple potential access-out-of bounds errors by moving range conditions before reads.
Multiple `for` loops across `libdecnumber` contain boolean expressions where
memory is accessed prior to checking if the pointer is still within a valid
range, which can lead to out-of-bounds reads.
This patch moves the range conditions to appear before the memory accesses in
each conjunction so that these expressions short-circuit instead of performing
an invalid read.
libdecnumber/ChangeLog
* In each `for` loop and `if` statement, all boolean expressions of the
form `*ptr == value && in_range(ptr)` have been changed to `in_range(ptr) &&
*ptr == value`.
Bootstrapped on x86_64-pc-linux-gnu with no regressions.
---
libdecnumber/decBasic.c | 20 ++--
libdecnumber/decCommon.c | 2 +-
libdecnumber/decNumber.c | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/libdecnumber/decBasic.c b/libdecnumber/decBasic.c
index 6319f66b25d..04833f2390d 100644
--- a/libdecnumber/decBasic.c
+++ b/libdecnumber/decBasic.c
@@ -341,7 +341,7 @@ static decFloat * decDivide(decFloat *result, const
decFloat *dfl,
for (;;) { /* inner loop -- calculate quotient unit */
/* strip leading zero units from acc (either there initially or */
/* from subtraction below); this may strip all if exactly 0 */
- for (; *msua==0 && msua>=lsua;) msua--;
+ for (; msua>=lsua && *msua==0;) msua--;
accunits=(Int)(msua-lsua+1); /* [maybe 0] */
/* subtraction is only necessary and possible if there are as */
/* least as many units remaining in acc for this iteration as */
@@ -515,7 +515,7 @@ static decFloat * decDivide(decFloat *result, const
decFloat *dfl,
/* (must also continue to original lsu for correct quotient length) */
if (lsua>acc+DIVACCLEN-DIVOPLEN) continue;
for (; msua>lsua && *msua==0;) msua--;
-if (*msua==0 && msua==lsua) break;
+if (msua==lsua && *msua==0) break;
} /* outer loop */
/* all of the original operand in acc has been covered at this point */
@@ -1543,8 +1543,8 @@ decFloat * decFloatAdd(decFloat *result,
umsd=acc+COFF+DECPMAX-1; /* so far, so zero */
if (ulsd>umsd) { /* more to check */
umsd++; /* to align after checked area */
- for (; UBTOUI(umsd)==0 && umsd+3msd)==0 && hi->msd+3lsd;) hi->msd+=4;
- for (; *hi->msd==0 && hi->msdlsd;) hi->msd++;
- for (; UBTOUI(lo->msd)==0 && lo->msd+3lsd;) lo->msd+=4;
- for (; *lo->msd==0 && lo->msdlsd;) lo->msd++;
+ for (; hi->msd+3lsd && UBTOUI(hi->msd)==0;) hi->msd+=4;
+ for (; hi->msdlsd && *hi->msd==0;) hi->msd++;
+ for (; lo->msd+3lsd && UBTOUI(lo->msd)==0;) lo->msd+=4;
+ for (; lo->msdlsd && *lo->msd==0;) lo->msd++;
/* if hi is zero then result will be lo (which has the smaller */
/* exponent), which also may need to be tested for zero for the */
@@ -2252,8 +2252,8 @@ decFloat * decFloatFMA(decFloat *result, const decFloat
*dfl,
/* all done except for the special IEEE 754 exact-zero-result */
/* rule (see above); while testing for zero, strip leading */
/* zeros (which will save decFinalize doing it) */
- for (; UBTOUI(lo->msd)==0 && lo->msd+3lsd;) lo->msd+=4;
- for (; *lo->msd==0 && lo->msdlsd;) lo->msd++;
+ for (; lo->msd+3lsd && UBTOUI(lo->msd)==0;) lo->msd+=4;
+ for (; lo->msdlsd && *lo->msd==0;) lo->msd++;
if (*lo->msd==0) { /* must be true zero (and diffsign) */
lo->sign=0;/* assume + */
if (set->round==DEC_ROUND_FLOOR) lo->sign=DECFLOAT_Sign;
diff --git a/libdecnumber/decCommon.c b/libdecnumber/decCommon.c
index 6f7563de6e6..1f9fe4a1935 100644
--- a/libdecnumber/decCommon.c
+++ b/libdecnumber/decCommon.c
@@ -276,7 +276,7 @@ static decFloat * decFinalize(decFloat *df, bcdnum *num,
/* [this is quite expensive] */
if (*umsd==0) {
for (; umsd+3exponent);
diff --git a/libdecnumber/decNumber.c b/libdecnumber/decNumber.c
index 094bc51c14a..89baef15749 100644
--- a/libdecnumber/decNumber.c
+++ b/libdecnumber/decNumber.c
@@ -4505,7 +4505,7 @@ static decNumber * decDivideOp(decNumber *res,
for (;;) { /* inner forever loop */
/* strip leading zero units [from either pre-adjust or from */
/* subtract last time around]. Leave at least one unit. */
- for (; *msu1==0 && msu1>var1; msu1--) var1units--;
+ for (; msu1>var1 && *msu1==0; msu1--) var1units--;
if (var1units
Re: [PATCH 1/2] libdecnumber: fixed undefined behavior in `decFloatFMA`
I've confirmed that these changes fix the error in MIRI, too. I'll post
an updated patch once I confirm that there aren't any regressions.
On Fri, Feb 2, 2024 at 10:38 AM Jakub Jelinek wrote:
> On Fri, Feb 02, 2024 at 04:32:09PM +0100, Jakub Jelinek wrote:
> > Anyway, I think all of
> > decBasic.c: for (; UBTOUI(umsd)==0 && umsd+3 > decBasic.c: for (; *umsd==0 && umsd > decBasic.c: for (; UBTOUI(hi->msd)==0 && hi->msd+3lsd;) hi->msd+=4;
> > decBasic.c: for (; *hi->msd==0 && hi->msdlsd;) hi->msd++;
> > decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3lsd;) lo->msd+=4;
> > decBasic.c: for (; *lo->msd==0 && lo->msdlsd;) lo->msd++;
> > decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3lsd;)
> lo->msd+=4;
> > decBasic.c: for (; *lo->msd==0 && lo->msdlsd;) lo->msd++;
> > decCommon.c: for (; *umsd==0 && umsd
> even more than that:
> decBasic.c: for (; *msua==0 && msua>=lsua;) msua--;
> decBasic.c:if (*msua==0 && msua==lsua) break;
> decCommon.c: if (*ulsd==0 && ulsd==umsd) { /* have zero */
> decNumber.c:for (; *msu1==0 && msu1>var1; msu1--) var1units--;
> too just from simple grep.
>
> Jakub
>
>
[PATCH 2/2] libdecnumber: fixed undefined behavior in decNumberGetBCD.
This patch fixes a minor instance of undefined behavior in libdecnumber. It was
discovered in the Rust bindings for libdecnumber (`dec`) using a custom version
of MIRI that can execute foreign functions.
On the last iteration of the `while` loop in `decNumberGetBCD`, the pointer
`up` will be incremented beyond the end of the allocation `dn->lsu` before the
assignment `u=*up`. This value does not affect the termination of the loop and
is never read again, so this isn't really an issue, but this patch prevent an
access out-of-bounds by only incrementing `up` if it is safe to do so.
Bootstrapped on x86_64-pc-linux-gnu with no regressions.
libdecnumber/ChangeLog
* decNumber.c: In `decNumberGetBCD`, only read from `dn->lsu` while the
pointer `up` is still within bounds.
---
libdecnumber/decNumber.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libdecnumber/decNumber.c b/libdecnumber/decNumber.c
index 0b6eb160fe3..094bc51c14a 100644
--- a/libdecnumber/decNumber.c
+++ b/libdecnumber/decNumber.c
@@ -3463,7 +3463,8 @@ uByte * decNumberGetBCD(const decNumber *dn, uByte *bcd) {
cut--;
if (cut>0) continue;/* more in this unit */
up++;
- u=*up;
+ if (ub > bcd)
+u=*up;
cut=DECDPUN;
}
#endif
--
2.39.3 (Apple Git-145)
[PATCH 1/2] libdecnumber: fixed undefined behavior in `decFloatFMA`
This patch fixes a minor instance of undefined behavior in libdecnumber. It was
discovered in the Rust bindings for libdecnumber (`dec`) using a custom version
of MIRI that can execute foreign functions.
Within the function `decFloatFMA`, the pointer `lo->msd` is initialized to
point to a byte array of size 56.
```
uByte acc[FMALEN];/* for multiplied coefficient in BCD */
...
ub=acc+FMALEN-1; /* where lsd of result will go */
...
lo->msd=ub+1;
lo->lsd=acc+FMALEN-1;
```
However, `lo->msd` is then offset in increments of 4, which leads to a read of
two bytes beyond the size of `acc`. This patch switches to reading in
increments of 2 instead of 4.
Bootstrapped on x86_64-pc-linux-gnu with no regressions.
libdecnumber/ChangeLog
* decBasic.c: Increment `lo->msd` by 2 instead of 4 in `decFloatFMA` to
avoid undefined behavior.
---
libdecnumber/decBasic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libdecnumber/decBasic.c b/libdecnumber/decBasic.c
index 6319f66b25d..3c8d71a2949 100644
--- a/libdecnumber/decBasic.c
+++ b/libdecnumber/decBasic.c
@@ -2023,6 +2023,7 @@ decFloat * decFloatFMA(decFloat *result, const decFloat
*dfl,
uInt carry;/* +1 for ten's complement and during add */
uByte *ub, *uh, *ul; /* work */
uInt uiwork; /* for macros */
+ uShort uswork;
/* handle all the special values [any special operand leads to a */
/* special result] */
@@ -2252,7 +2253,7 @@ decFloat * decFloatFMA(decFloat *result, const decFloat
*dfl,
/* all done except for the special IEEE 754 exact-zero-result */
/* rule (see above); while testing for zero, strip leading */
/* zeros (which will save decFinalize doing it) */
- for (; UBTOUI(lo->msd)==0 && lo->msd+3lsd;) lo->msd+=4;
+ for (; UBTOUS(lo->msd)==0 && lo->msd+1lsd;) lo->msd+=2;
for (; *lo->msd==0 && lo->msdlsd;) lo->msd++;
if (*lo->msd==0) { /* must be true zero (and diffsign) */
lo->sign=0;/* assume + */
--
2.39.3 (Apple Git-145)
