Better fix for snprintf non-termination on windows.

Better fix for snprintf non-termination on windows.

Replaces https://codereview.chromium.org/1062983002/
BUG=469244
R=brucedawson@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/d794018606dfdb62f87adf065c5ba48a4607d92b

[Tom Sepez, 2015-04-09 23:39:28]

tsepez@chromium.org changed reviewers:
+ brucedawson@chromium.org

[Tom Sepez, 2015-04-09 23:39:28]

Bruce, for review.

[Tom Sepez, 2015-04-10 17:59:45]

On 2015/04/09 23:39:28, Tom Sepez wrote:
> Bruce, for review.

Seems to build with void returns / casts.

[brucedawson, 2015-04-10 18:03:02]

https://codereview.chromium.org/1077903002/diff/80001/core/include/fxcrt/fx_s...
File core/include/fxcrt/fx_system.h (right):

https://codereview.chromium.org/1077903002/diff/80001/core/include/fxcrt/fx_s...
core/include/fxcrt/fx_system.h:143: #if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
Consider making times version limited. When VS 2015 comes out we can switch to
the standard versions, and eventually eliminate this platform-specific mess. You
can do this by adding:

&& _MSC_VER < 1900

This would apply to the definition as well as the declaration.

https://codereview.chromium.org/1077903002/diff/80001/core/include/fxcrt/fx_s...
core/include/fxcrt/fx_system.h:144: void FXSYS_snprintf(char *str, size_t size,
const char* fmt, ...);
Consider adding SAL annotations that let /analyze do parameter checking. The
Linux versions will catch most bugs, but some format-string mismatches are
compiler specific. To do this just put _Printf_format_string_ in front of 'const
char*'. This may require #include <sal.h> - I can check that.

Not crucial, but it can be valuable, especially for 64-bit builds.

https://codereview.chromium.org/1077903002/diff/80001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1077903002/diff/80001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:182: _vsnprintf(str, size, fmt, ap);
Consider a comment explaining why the void return type?

[Tom Sepez, 2015-04-10 18:23:40]

Please re-review.

https://codereview.chromium.org/1077903002/diff/80001/core/include/fxcrt/fx_s...
File core/include/fxcrt/fx_system.h (right):

https://codereview.chromium.org/1077903002/diff/80001/core/include/fxcrt/fx_s...
core/include/fxcrt/fx_system.h:143: #if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
On 2015/04/10 18:03:02, brucedawson wrote:
> Consider making times version limited. When VS 2015 comes out we can switch to
> the standard versions, and eventually eliminate this platform-specific mess.
You
> can do this by adding:
> 
> && _MSC_VER < 1900
> 
> This would apply to the definition as well as the declaration.

Done.

https://codereview.chromium.org/1077903002/diff/80001/core/include/fxcrt/fx_s...
core/include/fxcrt/fx_system.h:144: void FXSYS_snprintf(char *str, size_t size,
const char* fmt, ...);
On 2015/04/10 18:03:02, brucedawson wrote:
> Consider adding SAL annotations that let /analyze do parameter checking. The
> Linux versions will catch most bugs, but some format-string mismatches are
> compiler specific. To do this just put _Printf_format_string_ in front of
'const
> char*'. This may require #include <sal.h> - I can check that.
> 
> Not crucial, but it can be valuable, especially for 64-bit builds.

Done.

https://codereview.chromium.org/1077903002/diff/80001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1077903002/diff/80001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:182: _vsnprintf(str, size, fmt, ap);
On 2015/04/10 18:03:02, brucedawson wrote:
> Consider a comment explaining why the void return type?
Updated comment in the .h file.

[brucedawson, 2015-04-10 18:43:19]

https://codereview.chromium.org/1077903002/diff/140001/core/include/fxcrt/fx_...
File core/include/fxcrt/fx_system.h (right):

https://codereview.chromium.org/1077903002/diff/140001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_system.h:143: // that always NUL-terminate.
Say why we need to prevent use of the return value (_snprintf and snprintf have
different return values).

https://codereview.chromium.org/1077903002/diff/140001/core/src/fxcrt/fx_basi...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1077903002/diff/140001/core/src/fxcrt/fx_basi...
core/src/fxcrt/fx_basic_util.cpp:180: void FXSYS_vsnprintf(char *str, size_t
size, _Printf_format_string_ const char* fmt, va_list ap)
The _Printf_format_string annotation doesn't actually apply here since this
isn't a printf-style function, it's a vsnprintf style function (sorry).

[Tom Sepez, 2015-04-10 18:54:46]

Patchset #10 (id:180001) has been deleted

[Tom Sepez, 2015-04-10 18:54:56]

Patchset #9 (id:160001) has been deleted

[Tom Sepez, 2015-04-10 19:13:29]

Patchset #10 (id:220001) has been deleted

[Tom Sepez, 2015-04-10 19:13:35]

Patchset #9 (id:200001) has been deleted

[Tom Sepez, 2015-04-10 20:25:45]

Patchset #9 (id:240001) has been deleted

[Tom Sepez, 2015-04-10 20:26:11]

Patchset #9 (id:260001) has been deleted

[Tom Sepez, 2015-04-10 20:27:54]

Patchset #9 (id:280001) has been deleted

[Tom Sepez, 2015-04-10 20:30:25]

ps://codereview.chromium.org/1077903002/diff/140001/core/src/fxcrt/fx_basic_util.cpp#newcode180
> core/src/fxcrt/fx_basic_util.cpp:180: void FXSYS_vsnprintf(char *str, size_t
> size, _Printf_format_string_ const char* fmt, va_list ap)
> The _Printf_format_string annotation doesn't actually apply here since this
> isn't a printf-style function, it's a vsnprintf style function (sorry).
Done.

Please review the "unified diffs" if you're comfortable doing so while the
side-by-side is borked.  Thanks.

[Tom Sepez, 2015-04-10 20:40:20]

On 2015/04/10 20:30:25, Tom Sepez wrote:
>
ps://codereview.chromium.org/1077903002/diff/140001/core/src/fxcrt/fx_basic_util.cpp#newcode180
> > core/src/fxcrt/fx_basic_util.cpp:180: void FXSYS_vsnprintf(char *str, size_t
> > size, _Printf_format_string_ const char* fmt, va_list ap)
> > The _Printf_format_string annotation doesn't actually apply here since this
> > isn't a printf-style function, it's a vsnprintf style function (sorry).
> Done.
> 
> Please review the "unified diffs" if you're comfortable doing so while the
> side-by-side is borked.  Thanks.

... aaaaaaand we're back.

[brucedawson, 2015-04-10 21:33:10]

Excellent! Now lots of code is more secure, and this is more efficient than the
previous fix.

lgtm

[Tom Sepez, 2015-04-10 21:43:20]

Committed patchset #9 (id:300001) manually as
d794018606dfdb62f87adf065c5ba48a4607d92b (presubmit successful).