Erik de Castro Lopo
2014-Sep-18 12:14 UTC
[flac-dev] patch for win_utf8_io.c: vsnprintf_s vs. MinGW
lvqcl wrote:> Oops. It seems that vsnprintf_s isn't always available on MinGW platform: > MinGW declares this function only if MINGW_HAS_SECURE_API macro is defined. > That's because WinXP version of msvcrt.dll doesn't contain secure functions > like vsnprintf_s. > > Maybe it's better to revert vsnprintf_s to vsprintf or to use vnsprintf?Ok, we need to drop vsnprintf_s to support WinXP. I'd prefer vsnprintf over vsprintf but have no way of testing any of these options. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Erik de Castro Lopo wrote:>> Oops. It seems that vsnprintf_s isn't always available on MinGW platform: >> MinGW declares this function only if MINGW_HAS_SECURE_API macro is defined. >> That's because WinXP version of msvcrt.dll doesn't contain secure functions >> like vsnprintf_s. >> >> Maybe it's better to revert vsnprintf_s to vsprintf or to use vnsprintf? > > Ok, we need to drop vsnprintf_s to support WinXP. I'd prefer vsnprintf > over vsprintf but have no way of testing any of these options.I wrote a small program that fills a buffer[] with "abcdefghijklmnopqrstuvwxyz\0" pattern and then tries to write "0123456789" string into it. It calls either ret = vsnprintf_s(buffer, buf_size, _TRUNCATE, fmt, va); or ret = vsnprintf(buffer, buf_size, fmt, va); The results are: --------------------------------------------------------------------------- vsnprintf_s (MSVC, MinGW): buf_size = 8; ret = -1; buffer = "0123456" buf_size = 9; ret = -1; buffer = "01234567" buf_size = 10; ret = -1; buffer = "012345678" buf_size = 11; ret = 10; buffer = "0123456789" buf_size = 12; ret = 10; buffer = "0123456789" --------------------------------------------------------------------------- vsnprintf (MSVC, MinGW): buf_size = 8; ret = -1; buffer = "01234567ijklmnopqrstuvwxyz" buf_size = 9; ret = -1; buffer = "012345678jklmnopqrstuvwxyz" buf_size = 10; ret = 10; buffer = "0123456789klmnopqrstuvwxyz" buf_size = 11; ret = 10; buffer = "0123456789" buf_size = 12; ret = 10; buffer = "0123456789" --------------------------------------------------------------------------- vsnprintf (MinGW with "#define __USE_MINGW_ANSI_STDIO 1"): buf_size = 8; ret = 10; buffer = "0123456" buf_size = 9; ret = 10; buffer = "01234567" buf_size = 10; ret = 10; buffer = "012345678" buf_size = 11; ret = 10; buffer = "0123456789" buf_size = 12; ret = 10; buffer = "0123456789" --------------------------------------------------------------------------- By the way... according to MSDN page for CreateProcess() function <http://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx>: "The maximum length of [the command line] is 32,768 characters, including the Unicode terminating null character". UTF-8 uses up to 4 bytes for a character, so the maximum length of a temporary buffer for a call like flac_fprintf(stderr, "%s: ERROR: couldn't get block from chain\n", filename); should in theory exceed 4*32768 = 131072 bytes (plus some space for the rest of the message). Currently *printf_utf8() functions allocate 32768 bytes buffer.
> By the way... according to MSDN page for CreateProcess() function > <http://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx>: > "The maximum length of [the command line] is 32,768 characters, including the Unicode terminating null character". > > UTF-8 uses up to 4 bytes for a character, so the maximum length of > a temporary buffer for a call like > > flac_fprintf(stderr, "%s: ERROR: couldn't get block from chain\n", filename); > > should in theory exceed 4*32768 = 131072 bytes (plus some space for > the rest of the message). > > Currently *printf_utf8() functions allocate 32768 bytes buffer.More info (see also <http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx>): A file path is limited by 260 characters. But WINAPI also supports an extended-length path (it has "\\?\" prefix) which is limited by 32767 unicode characters: \\?\c:\path\to\filename.ext FLAC uses __wgetmainargs to obtain Unicode command line arguments. This function can also expand wildcards. But in this case it converts "\\?\c:\path\to\filename.ext" into just "\\filename.ext" (looks like a bug in MS code). So currently FLAC doesn't support very long paths. Disabling of wildcards expansion fixes this, and FLAC will be able to en- and decode files with an extended-length path. (what do you prefer: wildcards expansion OR extended-length path support?) ...On the other hand, _wstat64 function in MSVC runtime doesn't like '?' inside paths, and as a result grabbag__file_get_filesize() returns -1. So disabling of wildcards expansion will not result in 100% support of long file paths in FLAC.
lvqcl wrote:> I wrote a small program that fills a buffer[] with "abcdefghijklmnopqrstuvwxyz\0" > pattern and then tries to write "0123456789" string into it. > It calls either > ret = vsnprintf_s(buffer, buf_size, _TRUNCATE, fmt, va); > or > ret = vsnprintf(buffer, buf_size, fmt, va);<snip>> vsnprintf (MSVC, MinGW): > > buf_size = 8; ret = -1; buffer = "01234567ijklmnopqrstuvwxyz" > buf_size = 9; ret = -1; buffer = "012345678jklmnopqrstuvwxyz" > buf_size = 10; ret = 10; buffer = "0123456789klmnopqrstuvwxyz" > buf_size = 11; ret = 10; buffer = "0123456789" > buf_size = 12; ret = 10; buffer = "0123456789"This is actually the only one we can use. With a check on the the return value and a line or two of extra code, this function can made to behave resonably sensibly, even it its not possible to make it meet the requirements if the ISO C specs. I'm basically think of something like the (untested) diff below. Erik diff --git a/src/share/grabbag/snprintf.c b/src/share/grabbag/snprintf.c index 3a0661f..6a4e3ea 100644 --- a/src/share/grabbag/snprintf.c +++ b/src/share/grabbag/snprintf.c @@ -62,8 +62,11 @@ flac_snprintf(char *str, size_t size, const char *fmt, ...) va_start (va, fmt); #ifdef _MSC_VER - rc = vsnprintf_s (str, size, _TRUNCATE, fmt, va); - rc = (rc > 0) ? rc : (size == 0 ? 1024 : size * 2); + rc = vsnprintf (str, size, fmt, va); + if (rc < 0) { + rc = size - 1; + str [rc] = 0; + } #else rc = vsnprintf (str, size, fmt, va); #endif -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/