`wcsncpy` and `GetLocaleInfoEx` take length in number of characters, but `size` and `ret` counts number of bytes.
Previous commit changed a call to `GetLocaleInfoW` which counts lenght in `TCHAR`s (aka bytes), to the current `GetLocaleInfoEx`, which is probably the source of this confusion.
-- v2: msvcrt: fix out-of-bound access in create_locinfo
From: Yuxuan Shui yshui@codeweavers.com
Fixes: 24a2b625545f1875b5c3177f2b9 Signed-off-by: Yuxuan Shui yshui@codeweavers.com --- dlls/msvcrt/locale.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/dlls/msvcrt/locale.c b/dlls/msvcrt/locale.c index 9fa6ba76143..b82e5dec990 100644 --- a/dlls/msvcrt/locale.c +++ b/dlls/msvcrt/locale.c @@ -1251,18 +1251,18 @@ static __lc_time_data* create_time_data(WCHAR *sname) ret = 0; for(i=0; i<ARRAY_SIZE(time_data); i++) { cur->str.str[i] = &cur->data[ret]; - ret += GetLocaleInfoA(lcid, time_data[i], &cur->data[ret], size-ret); + ret += GetLocaleInfoA(lcid, time_data[i], &cur->data[ret], size-ret-sizeof(__lc_time_data)); } #if _MSVCR_VER == 0 || _MSVCR_VER >= 100 for(i=0; i<ARRAY_SIZE(time_data); i++) { cur->wstr.wstr[i] = (wchar_t*)&cur->data[ret]; ret += GetLocaleInfoEx(sname, time_data[i], - (wchar_t*)&cur->data[ret], size-ret)*sizeof(wchar_t); + (wchar_t*)&cur->data[ret], (size-ret-sizeof(__lc_time_data)) / sizeof(wchar_t))*sizeof(wchar_t); } #endif #if _MSVCR_VER >= 110 cur->locname = (wchar_t*)&cur->data[ret]; - wcsncpy((wchar_t *) &cur->data[ret], sname, size-ret); + wcsncpy((wchar_t *) &cur->data[ret], sname, (size-ret-sizeof(__lc_time_data)) / sizeof(wchar_t)); #else cur->lcid = lcid; #endif
oh i forgot one thing. `ret` didn't account for `sizeof(__lc_time_data)` but it should.
Victor Hermann Chiletto (@vitorhnn) commented about dlls/msvcrt/locale.c:
cur->str.str[i] = &cur->data[ret];
ret += GetLocaleInfoA(lcid, time_data[i], &cur->data[ret], size-ret);
ret += GetLocaleInfoA(lcid, time_data[i], &cur->data[ret], size-ret-sizeof(__lc_time_data));#if _MSVCR_VER == 0 || _MSVCR_VER >= 100 for(i=0; i<ARRAY_SIZE(time_data); i++) { cur->wstr.wstr[i] = (wchar_t*)&cur->data[ret]; ret += GetLocaleInfoEx(sname, time_data[i],
(wchar_t*)&cur->data[ret], size-ret)*sizeof(wchar_t);
(wchar_t*)&cur->data[ret], (size-ret-sizeof(__lc_time_data)) / sizeof(wchar_t))*sizeof(wchar_t);#endif #if _MSVCR_VER >= 110 cur->locname = (wchar_t*)&cur->data[ret];
- wcsncpy((wchar_t *) &cur->data[ret], sname, size-ret);
- wcsncpy((wchar_t *) &cur->data[ret], sname, (size-ret-sizeof(__lc_time_data)) / sizeof(wchar_t));
In retrospect, I should've used lstrcpynW instead of wcsncpy in all my patches. I'll submit a followup.
Thanks for catching this.
On Thu Jul 20 10:12:19 2023 +0000, Victor Hermann Chiletto wrote:
In retrospect, I should've used lstrcpynW instead of wcsncpy in all my patches. I'll submit a followup.
Why do you think that lstrcpynW should be used here?
On Thu Jul 20 11:38:28 2023 +0000, Piotr Caban wrote:
Why do you think that lstrcpynW should be used here?
Because this was a misuse of wcsncpy as it's (generally) meant for fixed size strings. wcsncpy here spends extra time writing zeros to fill the buffer which isn't useful, and it won't NULL terminate if for some reason the buffer isn't long enough (which should never happen here, but might happen in locale_to_sname). In fact, winbase.h has a pretty massive warning to never use wcsncpy.
lstrcpynW fits the bill much better here, as it always NULL terminates and it won't spend extra time nulling out the buffer. It would've hidden this issue though. I guess a basic wcscpy would've also been fine here, but not in locale_to_sname.
On Thu Jul 20 13:13:21 2023 +0000, Victor Hermann Chiletto wrote:
Thanks for catching this.
hi, i think i need a bit more time on this. so this change did fix a crash i was seeing last night, i tested with and without it several times. but this morning the crash came back. there might be something else, or this might not be what's causing it.
On Thu Jul 20 12:48:27 2023 +0000, Victor Hermann Chiletto wrote:
Because this was a misuse of wcsncpy as it's (generally) meant for fixed size strings. wcsncpy here spends extra time writing zeros to fill the buffer which isn't useful, and it won't NULL terminate if for some reason the buffer isn't long enough (which should never happen here, but might happen in locale_to_sname). In fact, winbase.h has a pretty massive warning to never use wcsncpy. lstrcpynW fits the bill much better here, as it always NULL terminates and it won't spend extra time nulling out the buffer. It would've hidden this issue though. I guess a basic wcscpy would've also been fine here, but not in locale_to_sname.
Sorry, my question was misleading. I was thinking about using wcscpy.
On Thu Jul 20 13:13:21 2023 +0000, Yuxuan Shui wrote:
hi, i think i need a bit more time on this. so this change did fix a crash i was seeing last night, i tested with and without it several times. but this morning the crash came back. there might be something else, or this might not be what's causing it.
Even if it doesn't fix your use case it's still a valid fix. Could you please approve my access request so I can push some minor changes to your patch.
On Thu Jul 20 13:31:17 2023 +0000, Piotr Caban wrote:
Even if it doesn't fix your use case it's still a valid fix. Could you please approve my access request so I can push some minor changes to your patch.
done, thanks
-
Piotr Caban (＠piotr) -
Victor Hermann Chiletto (＠vitorhnn)
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)