| Message ID | 20240202150905.42775-1-icmccorm@andrew.cmu.edu |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] libdecnumber: fixed undefined behavior in `decFloatFMA` | expand |
On Fri, Feb 02, 2024 at 10:09:05AM -0500, Ian McCormack wrote: > 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. Isn't 56 divisible by 4? Anyway, I think all of decBasic.c: for (; UBTOUI(umsd)==0 && umsd+3<ulsd;) umsd+=4; decBasic.c: for (; *umsd==0 && umsd<ulsd;) umsd++; decBasic.c: for (; UBTOUI(hi->msd)==0 && hi->msd+3<hi->lsd;) hi->msd+=4; decBasic.c: for (; *hi->msd==0 && hi->msd<hi->lsd;) hi->msd++; decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3<lo->lsd;) lo->msd+=4; decBasic.c: for (; *lo->msd==0 && lo->msd<lo->lsd;) lo->msd++; decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3<lo->lsd;) lo->msd+=4; decBasic.c: for (; *lo->msd==0 && lo->msd<lo->lsd;) lo->msd++; decCommon.c: for (; *umsd==0 && umsd<ulsd;) umsd++; look fishy, I'd expect all those conditions should first check if the index is still in range and only after && read from memory and compare against 0. I.e. like for (; umsd+3<ulsd && UBTOUI(umsd)==0;) umsd+=4; etc. Does that fix the issue too? Jakub
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<ulsd;) umsd+=4; > decBasic.c: for (; *umsd==0 && umsd<ulsd;) umsd++; > decBasic.c: for (; UBTOUI(hi->msd)==0 && hi->msd+3<hi->lsd;) hi->msd+=4; > decBasic.c: for (; *hi->msd==0 && hi->msd<hi->lsd;) hi->msd++; > decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3<lo->lsd;) lo->msd+=4; > decBasic.c: for (; *lo->msd==0 && lo->msd<lo->lsd;) lo->msd++; > decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3<lo->lsd;) lo->msd+=4; > decBasic.c: for (; *lo->msd==0 && lo->msd<lo->lsd;) lo->msd++; > decCommon.c: for (; *umsd==0 && umsd<ulsd;) 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
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 <jakub@redhat.com> 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<ulsd;) umsd+=4; > > decBasic.c: for (; *umsd==0 && umsd<ulsd;) umsd++; > > decBasic.c: for (; UBTOUI(hi->msd)==0 && hi->msd+3<hi->lsd;) hi->msd+=4; > > decBasic.c: for (; *hi->msd==0 && hi->msd<hi->lsd;) hi->msd++; > > decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3<lo->lsd;) lo->msd+=4; > > decBasic.c: for (; *lo->msd==0 && lo->msd<lo->lsd;) lo->msd++; > > decBasic.c: for (; UBTOUI(lo->msd)==0 && lo->msd+3<lo->lsd;) > lo->msd+=4; > > decBasic.c: for (; *lo->msd==0 && lo->msd<lo->lsd;) lo->msd++; > > decCommon.c: for (; *umsd==0 && umsd<ulsd;) 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 > >
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+3<lo->lsd;) lo->msd+=4; + for (; UBTOUS(lo->msd)==0 && lo->msd+1<lo->lsd;) lo->msd+=2; for (; *lo->msd==0 && lo->msd<lo->lsd;) lo->msd++; if (*lo->msd==0) { /* must be true zero (and diffsign) */ lo->sign=0; /* assume + */
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(-)