Enable positional parameters for base::vsnprintf and base::vswprintf on Windows.

Enable positional parameters for base::vsnprintf and base::vswprintf on Windows.


This is to avoid bugs like http://code.google.com/p/chromium/issues/detail?id=118064 in the future. Positional parameters are supported by the VC++ runtime via the set of "_p" flavored printf-like routines. MSDN claims that "By default the positional functions behave identically to the non position ones, if no positional formatting is present."

There is a little difference that require some attention though. Currently base::vsnprintf and base::vswprintf wrappers use "_s" functions (so called "security enhanced versions"). Judging by looking at the CRT code, _p functions implement the same checks as _s ones do. The base wrappers call the CRT routines so that count == (sizeOfBuffer - 1). This reduces most of the additional code in _vsnprintf_s (comparing to _vsprintf_p) to no-op. Namely:

1. When truncation happens the tail of the buffer is filled with a pattern:

    _SECURECRT__FILL_STRING(string, sizeInBytes, count + 1);

This does not happen in our case because sizeInBytes == count + 1.

2. The special case check shown below was never true since sizeInBytes != count in our case:

    if (count == 0 && string == NULL && sizeInBytes == 0)

3. The following checks in _vsnprintf_s are also implemented in _vsnprintf_helper:

_VALIDATE_RETURN(format != NULL, EINVAL, -1);
_VALIDATE_RETURN(string != NULL && sizeInBytes > 0, EINVAL, -1);

The only remaining difference between _vsnprintf_s and _vsprintf_p is that the former NULL-terminates the buffer and fills the rest a pattern if _vsnprintf_helper failed because of any reason other than truncation:

    string[0] = 0;
    _SECURECRT__FILL_STRING(string, sizeInBytes, 1);

This CL write NULL to the end of the buffer in any error case (truncation or other reason).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127788

[alexeypa (please no reviews), 2012-03-13 23:55:16]

Please take a look.

[willchan no longer on Chromium, 2012-03-14 09:47:12]

I'm inclined to approve this. Before I do so, let me voice a concern:
http://connect.microsoft.com/VisualStudio/feedback/details/98913/support-posi...
there's a performance cost here. Is this known / quantified? I'd
like to hear your thoughts on this first.

On Tue, Mar 13, 2012 at 4:55 PM, <alexeypa@chromium.org> wrote:

> Reviewers: willchan,
>
> Message:
> Please take a look.
>
> Description:
> Enable positional parameters for base::vsnprintf and base::vswprintf on
> Windows.
>
>
> This is to avoid bugs like
>
http://code.google.com/p/**chromium/issues/detail?id=**118064<http://code.goo...
the future.
> Positional parameters are supported by the VC++ runtime via the set of "_p"
> flavored printf-like routines. MSDN claims that "By default the positional
> functions behave identically to the non position ones, if no positional
> formatting is present."
>
> There is a little difference that require some attention though. Currently
> base::vsnprintf and base::vswprintf wrappers use "_s" functions (so called
> "security enhanced versions"). Judging by looking at the CRT code, _p
> functions
> implement the same checks as _s ones do. The base wrappers call the CRT
> routines
> so that count == (sizeOfBuffer - 1). This reduces most of the additional
> code in
> _vsnprintf_s (comparing to _vsprintf_p) to no-op. Namely:
>
> 1. When truncation happens the tail of the buffer is filled with a pattern:
>
>    _SECURECRT__FILL_STRING(**string, sizeInBytes, count + 1);
>
> This does not happen in our case because sizeInBytes == count + 1.
>
> 2. The special case check shown below was never true since sizeInBytes !=
> count
> in our case:
>
>    if (count == 0 && string == NULL && sizeInBytes == 0)
>
> 3. The following checks in _vsnprintf_s are also implemented in
> _vsnprintf_helper:
>
> _VALIDATE_RETURN(format != NULL, EINVAL, -1);
> _VALIDATE_RETURN(string != NULL && sizeInBytes > 0, EINVAL, -1);
>
> The only remaining difference between _vsnprintf_s and _vsprintf_p is that
> the
> former NULL-terminates the buffer and fills the rest a pattern if
> _vsnprintf_helper failed because of any reason other than truncation:
>
>    string[0] = 0;
>    _SECURECRT__FILL_STRING(**string, sizeInBytes, 1);
>
> This CL write NULL to the end of the buffer in any error case (truncation
> or
> other reason).
>
> Please review this at
https://chromiumcodereview.**appspot.com/9702002/<https://chromiumcodereview....
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M base/string_util_win.h
>
>
> Index: base/string_util_win.h
> diff --git a/base/string_util_win.h b/base/string_util_win.h
> index 8836f7442299c2252444cf4a2fb830**deb1620538..**
> cdf588f10bba3d13ee2ec54dedd5f4**557377f870 100644
> --- a/base/string_util_win.h
> +++ b/base/string_util_win.h
> @@ -35,9 +35,12 @@ inline int strncmp16(const char16* s1, const char16*
> s2, size_t count) {
>
>  inline int vsnprintf(char* buffer, size_t size,
>                      const char* format, va_list arguments) {
> -  int length = vsnprintf_s(buffer, size, size - 1, format, arguments);
> -  if (length < 0)
> -    return _vscprintf(format, arguments);
> +  int length = _vsprintf_p(buffer, size, format, arguments);
> +  if (length < 0) {
> +    if (size > 0)
> +      buffer[size - 1] = 0;
> +    return _vscprintf_p(format, arguments);
> +  }
>   return length;
>  }
>
> @@ -45,9 +48,12 @@ inline int vswprintf(wchar_t* buffer, size_t size,
>                      const wchar_t* format, va_list arguments) {
>   DCHECK(**IsWprintfFormatPortable(**format));
>
> -  int length = _vsnwprintf_s(buffer, size, size - 1, format, arguments);
> -  if (length < 0)
> -    return _vscwprintf(format, arguments);
> +  int length = _vswprintf_p(buffer, size, format, arguments);
> +  if (length < 0) {
> +    if (size > 0)
> +      buffer[size - 1] = 0;
> +    return _vscwprintf_p(format, arguments);
> +  }
>   return length;
>  }
>
>
>
>

[alexeypa (please no reviews), 2012-03-14 17:36:03]

On 2012/03/14 09:47:12, willchan wrote:
> I'm inclined to approve this. Before I do so, let me voice a concern:
>
http://connect.microsoft.com/VisualStudio/feedback/details/98913/support-posi...
> there's a performance cost here. Is this known / quantified? I'd
> like to hear your thoughts on this first.

The increased cost of printf_p comes from the fact that positional parameters
require two passes over the formatting string vs a single pass for plain
parameters as far as I can tell. The actual formatting routines _output_p_l and
_output_s_l are compiled from the same source, so comparing them is an easy job.
It appears that _output_p_l does a decent job of not increasing the cost for the
case when positional parameters are not used. It test whether the first
parameter is positional:

            if(format_type == FMT_TYPE_NOTSET)
            {
                /* We set the value of format_type when we hit the first type
specifier */
                if(_tcstol(format, &end_pos, 10) > 0 && (*end_pos ==
POSITION_CHAR))

If it is not a positional format_type is set to FMT_TYPE_NONPOSITIONAL that
effectively turns off the 2nd pass:

        if((pass == FORMAT_OUTPUT_PASS) && (format_type ==
FMT_TYPE_NONPOSITIONAL))
        {
            /* If in pass2, we still have format_type isn't positional, it means
            that we do not need a 2nd pass */
            break;
        }

It also makes the routine the invoke the same non positional formatting code as
_output_s_l. For instance:

        case ST_WIDTH:
            /* update width value */
            if (ch == _T('*')) {
                /* get width from arg list */
#ifdef POSITIONAL_PARAMETERS
                if(format_type == FMT_TYPE_NONPOSITIONAL)
                {
#endif  /* POSITIONAL_PARAMETERS */
                fldwidth = get_int_arg(&argptr);
#ifdef POSITIONAL_PARAMETERS
                }
                else
                {

I wrote a simple test measuring time _snprintf_s() and _sprintf_p() take to
format something like this:

    _snprintf_s(
      buffer, sizeof(buffer), sizeof(buffer) - 1,
      "Let's see how fast %s printf is comparing to %s: %d",
      "positional", "usual", 100);

I.e. this is the case when positional parameters are not used. I've got that on
x86 _p is about 4-6% slower than _s on that test. On x64 _p was 10-12% slower.

In the case when positional parameters were used _p was about 60% slower on x86
and 70% slower on x64.

I believe we can tolerate this cost. printf routines are not considered
performance critical anyway. They used quite often  in the code base but again
they are not used in performance critical manner.

[alexeypa (please no reviews), 2012-03-16 17:46:06]

ping.

[willchan no longer on Chromium, 2012-03-16 17:47:27]

Sorry. I agree with you analysis. Thanks for doing it and sorry for the
latency. LGTM.

On Fri, Mar 16, 2012 at 10:46 AM, <alexeypa@chromium.org> wrote:

> ping.
>
>
https://chromiumcodereview.**appspot.com/9702002/<https://chromiumcodereview....
>

[commit-bot: I haz the power, 2012-03-16 18:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9702002/1

[commit-bot: I haz the power, 2012-03-16 20:31:13]

Try job failure for 9702002-1 (retry) on win_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[alexeypa (please no reviews), 2012-03-17 00:26:44]

Could you please have another look?

The first version broke base_unittests!StringPrintfTest.Invalid that expected
NULL in the beginning of the buffer when an error occurs. I also added unit test
verifying that positional parameters are supported on all platforms. It seems to
be passing fine.

[alexeypa (please no reviews), 2012-03-20 15:44:55]

ping

[willchan no longer on Chromium, 2012-03-20 17:08:08]

lgtm

[commit-bot: I haz the power, 2012-03-20 17:08:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9702002/14001

[commit-bot: I haz the power, 2012-03-20 17:10:32]

Try job failure for 9702002-14001 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-03-20 18:55:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9702002/14001

[commit-bot: I haz the power, 2012-03-20 21:22:25]

Change committed as 127788