[PATCH 0/1] MR8256: include: vsnprintf should always write a null terminator.
Microsoft's documentation says:
The `vsnprintf` function always writes a null terminator, even if it truncates the output. When you use `_vsnprintf` and `_vsnwprintf`, the buffer is null-terminated only if there's room at the end (that is, if the number of characters to write is less than count).
For `_UCRT` and `_MSVCR_VER >= 140`, `vsnprintf` calls `__stdio_common_vsprintf` with `_CRT_INTERNAL_PRINTF_STANDARD_SNPRINTF_BEHAVIOR`, and everything works, a null byte is appended, everything is fine. If not, `vsnprintf` calls `_vsnprintf` directly and no null byte will be added if the buffer is not big enough. This happens in, for example, kernel32. And this breaks `wine_dbg_vsprintf`, which passes result of `vsnprintf` to `strlen` and goes out-of-bound. In msvcrt.spec, we also have `vsnprintf` as an alias of `_vsnprintf`, I didn't touch that since I don't know if that's deliberate. But this alias _is_ used (by e.g. winecrt0, i think?), if this one is also wrong then it might break other things. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8256
From: Yuxuan Shui <yshui(a)codeweavers.com> --- include/msvcrt/stdio.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/msvcrt/stdio.h b/include/msvcrt/stdio.h index c402a9a299e..44c2b816c60 100644 --- a/include/msvcrt/stdio.h +++ b/include/msvcrt/stdio.h @@ -458,7 +458,11 @@ _ACRTIMP int __cdecl vsprintf_s(char*,size_t,const char*,va_list) __WINE_CRT_PRI _ACRTIMP int __cdecl _vsnprintf(char*,size_t,const char*,va_list) __WINE_CRT_PRINTF_ATTR(3, 0); static inline int vsnprintf(char *buffer, size_t size, const char *format, va_list args) __WINE_CRT_PRINTF_ATTR(3, 0); static inline int vsnprintf(char *buffer, size_t size, const char *format, va_list args) -{ return _vsnprintf(buffer,size,format,args); } +{ + int r = _vsnprintf(buffer,size,format,args); + if (r>=size) buffer[size-1] = 0; + return r; +} _ACRTIMP int __cdecl _snscanf_l(const char*,size_t,const char*,_locale_t,...) __WINE_CRT_SCANF_ATTR(3, 5); _ACRTIMP int __cdecl _sscanf_l(const char *,const char*,_locale_t,...) __WINE_CRT_SCANF_ATTR(2, 4); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8256
Microsoft's documentation says:
that should be backed by a test probably. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8256#note_105917
Jinoh Kang (@iamahuman) commented about include/msvcrt/stdio.h:
_ACRTIMP int __cdecl _vsnprintf(char*,size_t,const char*,va_list) __WINE_CRT_PRINTF_ATTR(3, 0); static inline int vsnprintf(char *buffer, size_t size, const char *format, va_list args) __WINE_CRT_PRINTF_ATTR(3, 0); static inline int vsnprintf(char *buffer, size_t size, const char *format, va_list args) -{ return _vsnprintf(buffer,size,format,args); } +{ + int r = _vsnprintf(buffer,size,format,args); + if (r>=size) buffer[size-1] = 0; Missing check `size >= 1`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8256#note_106352
In msvcrt.spec, we also have vsnprintf as an alias of _vsnprintf, I didn't touch that since I don't know if that's deliberate. But this alias is used (by e.g. winecrt0, i think?), if this one is also wrong then it might break other things.
It's deliberate, that's the way it works on Windows. That's also why we don't want to change the inline definition. Any caller that can potentially link to msvcrt needs to be able to handle the old behavior. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8256#note_106763
On Mon Jun 16 23:51:13 2025 +0000, Alexandre Julliard wrote:
In msvcrt.spec, we also have vsnprintf as an alias of _vsnprintf, I didn't touch that since I don't know if that's deliberate. But this alias is used (by e.g. winecrt0, i think?), if this one is also wrong then it might break other things. It's deliberate, that's the way it works on Windows. That's also why we don't want to change the inline definition. Any caller that can potentially link to msvcrt needs to be able to handle the old behavior. thanks for the clarification! so should we instead add a null byte in `wine_dbg_vsprintf`?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8256#note_106800
On Mon Jun 16 23:51:13 2025 +0000, Yuxuan Shui wrote:
thanks for the clarification! so should we instead add a null byte in `wine_dbg_vsprintf`? Yes, and in other similar cases.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8256#note_106820
This merge request was closed by Yuxuan Shui. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8256
participants (5)
-
Alexandre Julliard (＠julliard) -
Jinoh Kang (＠iamahuman)
-
Paul Gofman (＠gofman)
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)