[PATCH v2 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. -- v2: msvcrt: Use _LDOUBLE type in exported functions. https://gitlab.winehq.org/wine/wine/-/merge_requests/7961
From: Yuxuan Shui <yshui(a)codeweavers.com> It fixes an ASan report due to write access to structure padding. --- dlls/msvcrt/msvcrt.h | 2 - dlls/msvcrt/string.c | 94 ++++++++++++++++++++++---------------------- 2 files changed, 47 insertions(+), 49 deletions(-) diff --git a/dlls/msvcrt/msvcrt.h b/dlls/msvcrt/msvcrt.h index 77170fff919..47cf70f1cc9 100644 --- a/dlls/msvcrt/msvcrt.h +++ b/dlls/msvcrt/msvcrt.h @@ -79,8 +79,6 @@ 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 __lc_time_data { union { const char *str[43]; diff --git a/dlls/msvcrt/string.c b/dlls/msvcrt/string.c index 90538533d95..6b4ffbf45b5 100644 --- a/dlls/msvcrt/string.c +++ b/dlls/msvcrt/string.c @@ -36,6 +36,12 @@ WINE_DEFAULT_DEBUG_CHANNEL(msvcrt); +struct MSVCRT__LDOUBLE +{ + ULONGLONG m; + unsigned short exp; +}; + /********************************************************************* * _mbsdup (MSVCRT.@) * _strdup (MSVCRT.@) @@ -474,25 +480,23 @@ int fpnum_double(struct fpnum *fp, double *d) #define LDBL_EXP_BITS 15 #define LDBL_MANT_BITS 64 -int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) +int fpnum_ldouble(struct fpnum *fp, struct MSVCRT__LDOUBLE *d) { if (fp->mod == FP_VAL_INFINITY) { - d->x80[0] = 0; - d->x80[1] = 0x80000000; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->m = 0x8000000000000000ull; + d->exp = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 1 << LDBL_EXP_BITS; return 0; } if (fp->mod == FP_VAL_NAN) { - d->x80[0] = ~0; - d->x80[1] = ~0; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->m = ~0ull; + d->exp = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 1 << LDBL_EXP_BITS; return 0; } @@ -500,31 +504,28 @@ int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) fp->m, fp->exp, fp->mod); if (!fp->m) { - d->x80[0] = 0; - d->x80[1] = 0; - d->x80[2] = 0; + d->m = 0; + d->exp = 0; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 1 << LDBL_EXP_BITS; return 0; } /* make sure that we don't overflow modifying exponent */ if (fp->exp > 1<<LDBL_EXP_BITS) { - d->x80[0] = 0; - d->x80[1] = 0x80000000; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->m = 0x8000000000000000ull; + d->exp = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 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->m = 0; + d->exp = 0; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 1 << LDBL_EXP_BITS; return ERANGE; } fp->exp += LDBL_MANT_BITS - 1; @@ -573,28 +574,25 @@ int fpnum_ldouble(struct fpnum *fp, MSVCRT__LDOUBLE *d) if (fp->exp >= (1<<LDBL_EXP_BITS)-1) { - d->x80[0] = 0; - d->x80[1] = 0x80000000; - d->x80[2] = (1 << LDBL_EXP_BITS) - 1; + d->m = 0x8000000000000000ull; + d->exp = (1 << LDBL_EXP_BITS) - 1; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 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->m = 0; + d->exp = 0; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 1 << LDBL_EXP_BITS; return ERANGE; } - d->x80[0] = fp->m; - d->x80[1] = fp->m >> 32; - d->x80[2] = fp->exp; + d->m = fp->m; + d->exp = fp->exp; if (fp->sign == -1) - d->x80[2] |= 1 << LDBL_EXP_BITS; + d->exp |= 1 << LDBL_EXP_BITS; return 0; } @@ -1501,9 +1499,10 @@ size_t CDECL strxfrm( char *dest, const char *src, size_t len ) /******************************************************************** * __STRINGTOLD_L (MSVCR80.@) */ -int CDECL __STRINGTOLD_L( MSVCRT__LDOUBLE *value, char **endptr, +int CDECL __STRINGTOLD_L( _LDOUBLE *value, char **endptr, const char *str, int flags, _locale_t locale ) { + struct MSVCRT__LDOUBLE *d = (struct MSVCRT__LDOUBLE *)value; pthreadlocinfo locinfo; const char *beg, *p; int err, ret = 0; @@ -1525,15 +1524,15 @@ int CDECL __STRINGTOLD_L( MSVCRT__LDOUBLE *value, char **endptr, if (endptr) *endptr = (p == beg ? (char*)str : (char*)p); if (p == beg) ret = 4; - err = fpnum_ldouble(&fp, value); - if (err) ret = (value->x80[2] & 0x7fff ? 2 : 1); + err = fpnum_ldouble(&fp, d); + if (err) ret = (d->exp & 0x7fff ? 2 : 1); return ret; } /******************************************************************** * __STRINGTOLD (MSVCRT.@) */ -int CDECL __STRINGTOLD( MSVCRT__LDOUBLE *value, char **endptr, const char *str, int flags ) +int CDECL __STRINGTOLD( _LDOUBLE *value, char **endptr, const char *str, int flags ) { return __STRINGTOLD_L( value, endptr, str, flags, NULL ); } @@ -1541,7 +1540,7 @@ int CDECL __STRINGTOLD( MSVCRT__LDOUBLE *value, char **endptr, const char *str, /******************************************************************** * _atoldbl_l (MSVCRT.@) */ -int CDECL _atoldbl_l( MSVCRT__LDOUBLE *value, char *str, _locale_t locale ) +int CDECL _atoldbl_l( _LDOUBLE *value, char *str, _locale_t locale ) { char *endptr; switch(__STRINGTOLD_L( value, &endptr, str, 0, locale )) @@ -1557,7 +1556,7 @@ int CDECL _atoldbl_l( MSVCRT__LDOUBLE *value, char *str, _locale_t locale ) */ int CDECL _atoldbl(_LDOUBLE *value, char *str) { - return _atoldbl_l( (MSVCRT__LDOUBLE*)value, str, NULL ); + return _atoldbl_l( value, str, NULL ); } /********************************************************************* @@ -2658,29 +2657,30 @@ struct _I10_OUTPUT_DATA { * Native sets last byte of data->str to '0' or '9', I don't know what * it means. Current implementation sets it always to '0'. */ -int CDECL I10_OUTPUT(MSVCRT__LDOUBLE ld80, int prec, int flag, struct _I10_OUTPUT_DATA *data) +int CDECL I10_OUTPUT(_LDOUBLE ld80, int prec, int flag, struct _I10_OUTPUT_DATA *data) { + struct MSVCRT__LDOUBLE *ld = (struct MSVCRT__LDOUBLE *)&ld80; struct fpnum num; double d; char format[8]; char buf[I10_OUTPUT_MAX_PREC+9]; /* 9 = strlen("0.e+0000") + '\0' */ char *p; - if ((ld80.x80[2] & 0x7fff) == 0x7fff) + if ((ld->exp & 0x7fff) == 0x7fff) { - if (ld80.x80[0] == 0 && ld80.x80[1] == 0x80000000) + if (ld->m == 0x8000000000000000ull) strcpy( data->str, "1#INF" ); else - strcpy( data->str, (ld80.x80[1] & 0x40000000) ? "1#QNAN" : "1#SNAN" ); + strcpy( data->str, (ld->m & 0x4000000000000000ull) ? "1#QNAN" : "1#SNAN" ); data->pos = 1; - data->sign = (ld80.x80[2] & 0x8000) ? '-' : ' '; + data->sign = (ld->exp & 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.m = ld80.x80[0] | ((ULONGLONG)ld80.x80[1] << 32); + num.sign = (ld->exp & 0x8000) ? -1 : 1; + num.exp = (ld->exp & 0x7fff) - 0x3fff - 63; + num.m = ld->m; num.mod = FP_ROUND_EVEN; fpnum_double( &num, &d ); TRACE("(%lf %d %x %p)\n", d, prec, flag, data); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7961
On Wed May 7 13:58:11 2025 +0000, Piotr Caban wrote:
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? Thank you! Applied.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7961#note_102761
This merge request was approved by Piotr Caban. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7961
participants (3)
-
Piotr Caban (＠piotr) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)