RFR: 8196882: VS2017 Hotspot Defined vsnprintf Function Causes C2084 Already Defined Compilation Error

[Kim Barrett]

> On Feb 26, 2018, at 10:55 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 
> 
> src/hotspot/os/windows/os_windows.cpp
> 
> Very minor nit: There is the theoretical possibility of _vsnprintf returning -1 for some reason other than errno. Documentation states "A return value of -1 indicates that an encoding error has occurred.". Since it does not say what the state of the output buffer is in that case, it may have been unchanged, in which case we would return undefined buffer content. To prevent this, maybe we could set the first buffer byte to \0 before invoking vsnprintf (if len > 0).
> 
> However, I admit this is very far fetched. Will probably never happen, at least, I have never seen it. So, I leave it to you if you do this or not.

This is related to my earlier "interesting point"; encoding errors
only appear to be possible when dealing with wide characters or
strings, which I don't think would ever happen for HotSpot usage.  I
agree the state of the output buffer does not seem to be well defined
in the case where an encoding error occured.  The C99 description of
snprintf looks to me like it might be trying to say the output is NUL
terminated even in that case, but I don't think it unambiguously
succeeds.

However, pre-setting the first byte to NUL doesn't really help; that
byte may have been overwritten before the encoding error is detected.
We can instead set the last byte of the buffer to NUL when len > 0 and
result < 0.  (We could do so without checking result, but that makes
the gtest's test for stray writes a little more complicated.) I'm
making that change, mostly in case we get here via the jio_ functions.

Note that the documentation for _vsnprintf (or vsnprintf, prior to
VS2015) makes no mention (that I could find) of encoding errors as a
possible reason for a negative return value.

(Interestingly, the Java Access Bridge (jdk.accessibility) native
windows code uses wide char/string format directives, and appears to
in at least some cases write them using bare vsnprintf, and that's
irrespective of which VS version is being used.)

> 
> ---
> 
> test/hotspot/gtest/runtime/test_os.cpp
> 
> - check_buffer is used to check prefix and suffix range for stray writes? I think this may be overthinking it a bit, I would not expect strays beyond buf - 1 and buf + len, in which case you would not need the check_buffer.

Using check_buffer demonstrated behavior differences between some
versions of these changes (because of NULing out buf[len-1]).  I'm
inclinded to keep it.

> - By initializing buffer with \0 you will miss a faulty os::snprintf() failing to write the terminating zero, no? I would use a different value.

The fill value for checking is not '\0', it's '0'.  I'll change that
to make it more obviously different.

> Otherwise, looks good to me.

It seems I sent out the open.02 webrev with a few final edits in
shared code that were only tested locally, and failed to build on some
platforms. Windows complained about an implicit narrowing conversion
in ostream.cpp.  For Solaris, a C linkage function has a different
type than a C++ linkage function with the same signature, so for testing
jio_snprintf I changed test_sprintf to a function template with the
print function type deduced.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8196882/open.03/
incr: http://cr.openjdk.java.net/~kbarrett/8196882/open.03.inc/


> Best Regards, Thomas
> 
> On Sun, Feb 25, 2018 at 11:42 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> > On Feb 25, 2018, at 5:37 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> > Based on discussion, I've changed the new os::vsnprintf and
> > os::snprintf to conform to C99.  […]
> >
> > Also changed jio_vsnprintf to use os::vsnprintf, reverting some of the
> > ealier change.
> 
> Just to be clear, the jio_vsnprintf behavior has not been changed.  It’s just been
> reimplemented in terms of os::vsnprintf rather than directly using ::vsnprintf and
> trying to account for its platform variations.