Re: [flac-dev] vsnprintf_s and vsnprintf

[JonY]

On 9/21/2014 08:17, lvqcl wrote:
> JonY wrote:
> 
>> You could just request mingw to include a vsnprintf_s implementation for
>> XP and earlier, mingw-w64 has already done so.
> 
> I use MinGW-w64: it's XhmikosR's MSYS that contains
> "MinGW-w64 build of GCC/Binutils for Windows" from nevcairiel.
> It has mingw-w64 3.1.0 inside.
> 
> But I thought that MinGW-w64 links with msvcrt.dll and
> uses vsnprintf_s from it. Are you sure that MinGW-w64
> has its own independent implementation of vsnprintf_s?

No, it falls back on an internal implementation if it isn't found in
MSVCRT. And furthermore, I retract my statement after looking into the
implementation, it is just calling vsprintf while disregarding the size.





signature.asc
Description: OpenPGP digital signature
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[lvqcl]

lvqcl wrote:

> I would like to know opinions of other people... Especially because I'm not
> very familiar with MinGW. Anyone?

Is it safe to use __mingw_vsnprintf? If yes, then flac_snprintf can look like:

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 = something_that_makes_sense;
#elif defined __MINGW32__
rc = __mingw_vsnprintf (str, size, fmt, va);
#else
rc = vsnprintf (str, size, fmt, va);
#endif
va_end (va);

return rc;
}
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[lvqcl]

JonY wrote:

> You could just request mingw to include a vsnprintf_s implementation for
> XP and earlier, mingw-w64 has already done so.

I use MinGW-w64: it's XhmikosR's MSYS that contains
"MinGW-w64 build of GCC/Binutils for Windows" from nevcairiel.
It has mingw-w64 3.1.0 inside.

But I thought that MinGW-w64 links with msvcrt.dll and
uses vsnprintf_s from it. Are you sure that MinGW-w64
has its own independent implementation of vsnprintf_s?
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[JonY]

On 9/21/2014 06:58, lvqcl wrote:
> Erik de Castro Lopo wrote:
> 
>>> 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?
> 
> No, flac.exe compiled with MSVS 2013 (and dynamically linked with MSVC 
> runtime) will
> simply require msvcr120.dll to run. On both OSes.
> 

You could just request mingw to include a vsnprintf_s implementation for
XP and earlier, mingw-w64 has already done so.




signature.asc
Description: OpenPGP digital signature
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[lvqcl]

Erik de Castro Lopo wrote:

>> 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?

No, flac.exe compiled with MSVS 2013 (and dynamically linked with MSVC runtime) 
will
simply require msvcr120.dll to run. On both OSes.

> 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.

I would like to know opinions of other people... Especially because I'm not
very familiar with MinGW. Anyone?


P.S. There are 3 places where flac uses vsnprintf:
1) flac_snprintf inside src/share/grabbag/snprintf.c
2) local_snprintf inside src/libFLAC/metadata_iterators.c -- but it's a full 
copy of flac_snprintf
3) printf_utf8/fprintf_utf8/vfprintf_utf8 inside 
src/share/win_utf8_io/win_utf8_io.c
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[Erik de Castro Lopo]

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/
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[lvqcl]

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;
}
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[Erik de Castro Lopo]

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);



>   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/
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] vsnprintf_s and vsnprintf

[lvqcl]

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
:
"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.
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev