[PATCH v2 0/1] MR3358: msvcrt: fix out-of-bound access in create_locinfo
`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 https://gitlab.winehq.org/wine/wine/-/merge_requests/3358
From: Yuxuan Shui <yshui(a)codeweavers.com> Fixes: 24a2b625545f1875b5c3177f2b9 Signed-off-by: Yuxuan Shui <yshui(a)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 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3358
oh i forgot one thing. `ret` didn't account for `sizeof(__lc_time_data)` but it should. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39566
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39580
Thanks for catching this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39581
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?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39596
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39599
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39608
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39609
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39611
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
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3358#note_39617
participants (4)
-
Piotr Caban (＠piotr) -
Victor Hermann Chiletto (＠vitorhnn)
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)