[PATCH 0/1] MR7961: msvcrt: Fix buffer overflow in __STRINGTOLD_L
__STRINGTOLD_L expects a pointer to a `_LDOUBLE` as its first argument, which we internally declare as `MSVCRT__LDOUBLE`. Problem is, `_LDOUBLE` is 10 bytes long, whereas `MSVCRT__LDOUBLE` is 12 bytes long (ULONG * 3). Writing into the third ULONG is technically a buffer overflow. Declare `MSVCRT__LDOUBLE` instead as 2 ULONGs plus a USHORT. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7961
From: Yuxuan Shui <yshui(a)codeweavers.com> __STRINGTOLD_L expects a pointer to a `_LDOUBLE` as its first argument, which we internally declare as `MSVCRT__LDOUBLE`. Problem is, `_LDOUBLE` is 10 bytes long, whereas `MSVCRT__LDOUBLE` is 12 bytes long (ULONG * 3). Writing into the third ULONG is technically a buffer overflow. Declare `MSVCRT__LDOUBLE` instead as 2 ULONGs plus a USHORT. --- dlls/msvcrt/msvcrt.h | 2 +- dlls/msvcrt/string.c | 42 +++++++++++++++++++++--------------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/dlls/msvcrt/msvcrt.h b/dlls/msvcrt/msvcrt.h index 77170fff919..fdd14d390d1 100644 --- a/dlls/msvcrt/msvcrt.h +++ b/dlls/msvcrt/msvcrt.h @@ -79,7 +79,7 @@ void __cdecl terminate(void); typedef void (__cdecl *MSVCRT_security_error_handler)(int, void *); -typedef struct {ULONG x80[3];} MSVCRT__LDOUBLE; /* Intel 80 bit FP format has sizeof() 12 */ +typedef struct {ULONG x80[2]; USHORT x80_2;} MSVCRT__LDOUBLE; typedef struct __lc_time_data { union { diff --git a/dlls/msvcrt/string.c b/dlls/msvcrt/string.c index 90538533d95..1a1ef52b9f2 100644 --- a/dlls/msvcrt/string.c +++ b/dlls/msvcrt/string.c @@ -480,9 +480,9 @@ int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) { d->x80[0] = 0; d->x80[1] = 0x80000000; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->x80_2 = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return 0; } @@ -490,9 +490,9 @@ int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) { d->x80[0] = ~0; d->x80[1] = ~0; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->x80_2 = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return 0; } @@ -502,9 +502,9 @@ int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) { d->x80[0] = 0; d->x80[1] = 0; - d->x80[2] = 0; + d->x80_2 = 0; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return 0; } @@ -513,18 +513,18 @@ int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) { d->x80[0] = 0; d->x80[1] = 0x80000000; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->x80_2 = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return ERANGE; } if (fp->exp < -(1<<LDBL_EXP_BITS)) { d->x80[0] = 0; d->x80[1] = 0; - d->x80[2] = 0; + d->x80_2 = 0; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return ERANGE; } fp->exp += LDBL_MANT_BITS - 1; @@ -575,26 +575,26 @@ int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) { d->x80[0] = 0; d->x80[1] = 0x80000000; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->x80_2 = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return ERANGE; } if (!fp->m || fp->exp < 0) { d->x80[0] = 0; d->x80[1] = 0; - d->x80[2] = 0; + d->x80_2 = 0; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return ERANGE; } d->x80[0] = fp->m; d->x80[1] = fp->m >> 32; - d->x80[2] = fp->exp; + d->x80_2 = fp->exp; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->x80_2 |= 1 << LDBL_EXP_BITS; return 0; } @@ -1526,7 +1526,7 @@ int CDECL __STRINGTOLD_L( MSVCRT__LDOUBLE *value, char **endptr, if (p == beg) ret = 4; err = fpnum_ldouble(&fp, value); - if (err) ret = (value->x80[2] & 0x7fff ? 2 : 1); + if (err) ret = (value->x80_2 & 0x7fff ? 2 : 1); return ret; } @@ -2666,20 +2666,20 @@ int CDECL I10_OUTPUT(MSVCRT__LDOUBLE ld80, int prec, int flag, struct _I10_OUTPU char buf[I10_OUTPUT_MAX_PREC+9]; /* 9 = strlen("0.e+0000") + '\0' */ char *p; - if ((ld80.x80[2] & 0x7fff) == 0x7fff) + if ((ld80.x80_2 & 0x7fff) == 0x7fff) { if (ld80.x80[0] == 0 && ld80.x80[1] == 0x80000000) strcpy( data->str, "1#INF" ); else strcpy( data->str, (ld80.x80[1] & 0x40000000) ? "1#QNAN" : "1#SNAN" ); data->pos = 1; - data->sign = (ld80.x80[2] & 0x8000) ? '-' : ' '; + data->sign = (ld80.x80_2 & 0x8000) ? '-' : ' '; data->len = strlen(data->str); return 0; } - num.sign = (ld80.x80[2] & 0x8000) ? -1 : 1; - num.exp = (ld80.x80[2] & 0x7fff) - 0x3fff - 63; + num.sign = (ld80.x80_2 & 0x8000) ? -1 : 1; + num.exp = (ld80.x80_2 & 0x7fff) - 0x3fff - 63; num.m = ld80.x80[0] | ((ULONGLONG)ld80.x80[1] << 32); num.mod = FP_ROUND_EVEN; fpnum_double( &num, &d ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7961
Isn't the struct still 12 bytes with this change, due to padding? I don't know if that mismatch causes any trouble, but it's worth asking about, at least. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7961#note_102470
On Sun May 4 23:35:24 2025 +0000, Alfred Agrell wrote:
Isn't the struct still 12 bytes with this change, due to padding? I don't know if that mismatch causes any trouble, but it's worth asking about, at least. `sizeof(MSVCRT__LDOUBLE)` will still be 12 bytes due to trailing padding, but we wouldn't be reading or writing the last 2 bytes anymore, so it's fine.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7961#note_102483
I was thinking about something like [0001-msvcrt-Use-_LDOUBLE-type-in-exported-functions.patch](/uploads/5b8330532e8ef0e472babe1e268d9703/0001-msvcrt-Use-_LDOUBLE-type-in-exported-functions.patch) when we talked about fixing it. Could you please update the MR with this patch if it looks good for you? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7961#note_102498
participants (5)
-
Alex Henrie (＠alexhenrie) -
Alfred Agrell (＠Alcaro)
-
Piotr Caban (＠piotr)
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)