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

[Thomas Stüfe]

Hi Kim,

On Tue, Feb 27, 2018 at 8:52 AM, Kim Barrett <kim.barrett at oracle.com> wrote:

> > 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.
>
>
That makes sense.


> 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.)
>
>
Where? I took a short look and did not find it.

However, I found a number of wcsncpy() with no truncation handling, so, no
zero-termination upon truncation (I think, but I may be mistaken, just had
a very quick look).



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


> > - 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.
>
>
Yes, that is clearer, thanks.


> > 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/
>
>
>
All looks well. Thank you.

Best Regards, Thomas


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