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/
Erik de Castro Lopo wrote:>> 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.Why? We can use vsnprintf_s for MSVS, and vsnprintf for MinGW. MSVS version of vsnprintf_s is located inside (statically linked) msvcp???.lib or (dynamically linked) msvcp???.dll. They are part of MSVS runtime, and compatible with WinXP. So it is safe to use it in FLAC. OTOH MinGW uses vsnprintf_s from msvcrt.dll file that belongs to Windows. WinXP version of msvcrt.dll doesn't have vsnprintf_s. So we have to use vsnprintf when the compiler is MinGW.> 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. > > @@ -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>> buf_size = 10; ret = 10; buffer = "0123456789klmnopqrstuvwxyz"As you see, MSVC version of vsnprintf can return positive value even when it didn't write '\0' at the end. So if (rc < 0) { ... str [...] = 0; } is not enough. What about this version -- int flac_snprintf(char *str, size_t size, const char *fmt, ...) { va_list va; int rc; va_start (va, fmt); #if defined _MSC_VER rc = vsnprintf_s (str, size, _TRUNCATE, fmt, va); if (rc < 0) rc = size - 1; #elif defined __MINGW32__ && (!defined __USE_MINGW_ANSI_STDIO || __USE_MINGW_ANSI_STDIO == 0) rc = vsnprintf (str, size, fmt, va); if (rc < 0 || rc == size) { rc = size - 1; str [rc] = '\0'; /* assert(size > 0) */ } #else rc = vsnprintf (str, size, fmt, va); #endif va_end (va); return rc; } Of course it is also possible to get rid of vsnprintf_s: int flac_snprintf(char *str, size_t size, const char *fmt, ...) { va_list va; int rc; va_start (va, fmt); #if defined _MSC_VER || (defined __MINGW32__ && (!defined __USE_MINGW_ANSI_STDIO || __USE_MINGW_ANSI_STDIO == 0)) rc = vsnprintf (str, size, fmt, va); if (rc < 0 || rc == size) { rc = size - 1; str [rc] = '\0'; /* assert(size > 0) */ } #else rc = vsnprintf (str, size, fmt, va); #endif va_end (va); return rc; } Or forget about __USE_MINGW_ANSI_STDIO: int flac_snprintf(char *str, size_t size, const char *fmt, ...) { va_list va; int rc; va_start (va, fmt); #if defined _MSC_VER || defined __MINGW32__ rc = vsnprintf (str, size, fmt, va); if (rc < 0 || rc == size) { rc = size - 1; str [rc] = '\0'; /* assert(size > 0) */ } #else rc = vsnprintf (str, size, fmt, va); #endif va_end (va); return rc; }
lvqcl wrote:> Why? We can use vsnprintf_s for MSVS, and vsnprintf for MinGW. > > MSVS version of vsnprintf_s is located inside (statically linked) msvcp???.lib > or (dynamically linked) msvcp???.dll. They are part of MSVS runtime, and compatible > with WinXP. So it is safe to use it in FLAC.Oh, ok. I missed this bit. I know so very little about Windows. However, if you compile flac on say Win7 using vsnprintf_s and then take the dynamically compiled binary to WinXP it will fail, right?> As you see, MSVC version of vsnprintf can return positive value > even when it didn't write '\0' at the end. So > if (rc < 0) { ... str [...] = 0; } > is not enough.Like I said, untested. It was the first thing that came into my head version of the code :-).> What about this version -- > > int flac_snprintf(char *str, size_t size, const char *fmt, ...) > { > va_list va; > int rc; > > va_start (va, fmt); > > #if defined _MSC_VER > rc = vsnprintf_s (str, size, _TRUNCATE, fmt, va); > if (rc < 0) > rc = size - 1; > #elif defined __MINGW32__ && (!defined __USE_MINGW_ANSI_STDIO || __USE_MINGW_ANSI_STDIO == 0) > rc = vsnprintf (str, size, fmt, va); > if (rc < 0 || rc == size) { > rc = size - 1; > str [rc] = '\0'; /* assert(size > 0) */ > } > #else > rc = vsnprintf (str, size, fmt, va); > #endif > va_end (va); > > return rc; > }I like that but ....> Or forget about __USE_MINGW_ANSI_STDIO: > > int flac_snprintf(char *str, size_t size, const char *fmt, ...) > { > va_list va; > int rc; > > va_start (va, fmt); > > #if defined _MSC_VER || defined __MINGW32__ > rc = vsnprintf (str, size, fmt, va); > if (rc < 0 || rc == size) { > rc = size - 1; > str [rc] = '\0'; /* assert(size > 0) */ > } > #else > rc = vsnprintf (str, size, fmt, va); > #endif > va_end (va); > > return rc; > }This one has the advantage of simplicity, there are only two things to test. Either of those two are fine. You choose and send a patch. I'll write a unit test for flac_snprintf to capture our assumptions. Cheers, Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/