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

Summary: Add os::vsnprintf and os::snprintf.
Reviewed-by: kbarrett, lfoltan, stuefe, mlarsson

src/os/posix/vm/os_posix.cpp

 /*
-* Copyright (c) 1999, 2015, Oracle and/or its affiliates. All rights reserved.
+* Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -166,6 +166,16 @@ char* os::reserve_memory_aligned(size_t size, size_t alignment) {
   return aligned_base;
 }
 
+int os::vsnprintf(char* buf, size_t len, const char* fmt, va_list args) {
+  int result = ::vsnprintf(buf, len, fmt, args);
+  // If an encoding error occurred (result < 0) then it's not clear
+  // whether the buffer is NUL terminated, so ensure it is.
+  if ((result < 0) && (len > 0)) {
+    buf[len - 1] = '\0';
+  }
+  return result;
+}
+
 void os::Posix::print_load_average(outputStream* st) {
   st->print("load average:");
   double loadavg[3];

src/os/windows/vm/os_windows.cpp

@@ -1837,6 +1837,42 @@ void os::print_siginfo(outputStream *st, void *siginfo) {
   st->cr();
 }
 
+
+int os::vsnprintf(char* buf, size_t len, const char* fmt, va_list args) {
+#if _MSC_VER >= 1900
+  // Starting with Visual Studio 2015, vsnprint is C99 compliant.
+  int result = ::vsnprintf(buf, len, fmt, args);
+  // If an encoding error occurred (result < 0) then it's not clear
+  // whether the buffer is NUL terminated, so ensure it is.
+  if ((result < 0) && (len > 0)) {
+    buf[len - 1] = '\0';
+  }
+  return result;
+#else
+  // Before Visual Studio 2015, vsnprintf is not C99 compliant, so use
+  // _vsnprintf, whose behavior seems to be *mostly* consistent across
+  // versions.  However, when len == 0, avoid _vsnprintf too, and just
+  // go straight to _vscprintf.  The output is going to be truncated in
+  // that case, except in the unusual case of empty output.  More
+  // importantly, the documentation for various versions of Visual Studio
+  // are inconsistent about the behavior of _vsnprintf when len == 0,
+  // including it possibly being an error.
+  int result = -1;
+  if (len > 0) {
+    result = _vsnprintf(buf, len, fmt, args);
+    // If output (including NUL terminator) is truncated, the buffer
+    // won't be NUL terminated.  Add the trailing NUL specified by C99.
+    if ((result < 0) || (result >= (int) len)) {
+      buf[len - 1] = '\0';
+    }
+  }
+  if (result < 0) {
+    result = _vscprintf(fmt, args);
+  }
+  return result;
+#endif // _MSC_VER dispatch
+}
+
 void os::print_signal_handlers(outputStream* st, char* buf, size_t buflen) {
   // do nothing
 }

src/share/vm/gc_implementation/g1/g1GCPhaseTimes.cpp

@@ -41,13 +41,13 @@ private:
   int _cur;
 
   void vappend(const char* format, va_list ap)  ATTRIBUTE_PRINTF(2, 0) {
-    int res = vsnprintf(&_buffer[_cur], BUFFER_LEN - _cur, format, ap);
-    if (res != -1) {
-      _cur += res;
-    } else {
+    int res = os::vsnprintf(&_buffer[_cur], BUFFER_LEN - _cur, format, ap);
+    if (res > BUFFER_LEN) {
       DEBUG_ONLY(warning("buffer too small in LineBuffer");)
       _buffer[BUFFER_LEN -1] = 0;
       _cur = BUFFER_LEN; // vsnprintf above should not add to _buffer if we are called again
+    } else if (res != -1) {
+      _cur += res;
     }
   }
 

src/share/vm/oops/generateOopMap.cpp

 /*
- * Copyright (c) 1997, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -29,6 +29,7 @@
 #include "oops/symbol.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/java.hpp"
+#include "runtime/os.hpp"
 #include "runtime/relocator.hpp"
 #include "utilities/bitMap.inline.hpp"
 #include "prims/methodHandles.hpp"
@@ -2133,10 +2134,10 @@ void GenerateOopMap::compute_map(TRAPS) {
 void GenerateOopMap::error_work(const char *format, va_list ap) {
   _got_error = true;
   char msg_buffer[512];
-  vsnprintf(msg_buffer, sizeof(msg_buffer), format, ap);
+  os::vsnprintf(msg_buffer, sizeof(msg_buffer), format, ap);
   // Append method name
   char msg_buffer2[512];
-  jio_snprintf(msg_buffer2, sizeof(msg_buffer2), "%s in method %s", msg_buffer, method()->name()->as_C_string());
+  os::snprintf(msg_buffer2, sizeof(msg_buffer2), "%s in method %s", msg_buffer, method()->name()->as_C_string());
   _exception = Exceptions::new_exception(Thread::current(),
                 vmSymbols::java_lang_LinkageError(), msg_buffer2);
 }
@@ -2154,7 +2155,7 @@ void GenerateOopMap::verify_error(const char *format, ...) {
   _got_error = true;
   // Append method name
   char msg_buffer2[512];
-  jio_snprintf(msg_buffer2, sizeof(msg_buffer2), "%s in method %s", msg,
+  os::snprintf(msg_buffer2, sizeof(msg_buffer2), "%s in method %s", msg,
                method()->name()->as_C_string());
   _exception = Exceptions::new_exception(Thread::current(),
                 vmSymbols::java_lang_LinkageError(), msg_buffer2);

src/share/vm/prims/jni.cpp

@@ -33,6 +33,7 @@
 #include "classfile/vmSymbols.hpp"
 #include "interpreter/linkResolver.hpp"
 #include "utilities/macros.hpp"
+#include "utilities/ostream.hpp"
 #if INCLUDE_ALL_GCS
 #include "gc_implementation/g1/g1SATBCardTableModRefBS.hpp"
 #endif // INCLUDE_ALL_GCS
@@ -5127,6 +5128,7 @@ void execute_internal_vm_tests() {
     run_unit_test(GuardedMemory::test_guarded_memory());
     run_unit_test(AltHashing::test_alt_hash());
     run_unit_test(test_loggc_filename());
+    run_unit_test(test_snprintf());
     run_unit_test(TestNewSize_test());
     run_unit_test(TestKlass_test());
     run_unit_test(Test_linked_list());

src/share/vm/prims/jvm.cpp

@@ -2915,16 +2915,12 @@ extern "C" {
 
 ATTRIBUTE_PRINTF(3, 0)
 int jio_vsnprintf(char *str, size_t count, const char *fmt, va_list args) {
-  // see bug 4399518, 4417214
+  // Reject count values that are negative signed values converted to
+  // unsigned; see bug 4399518, 4417214
   if ((intptr_t)count <= 0) return -1;
 
-  int result = vsnprintf(str, count, fmt, args);
-  // Note: on truncation vsnprintf(3) on Unix returns number of
-  // characters which would have been written had the buffer been large
-  // enough; on Windows, it returns -1. We handle both cases here and
-  // always return -1, and perform null termination.
-  if ((result > 0 && (size_t)result >= count) || result == -1) {
-    str[count - 1] = '\0';
+  int result = os::vsnprintf(str, count, fmt, args);
+  if (result > 0 && (size_t)result >= count) {
     result = -1;
   }
 

src/share/vm/runtime/os.cpp

@@ -108,6 +108,14 @@ static time_t get_timezone(const struct tm* time_struct) {
 #endif
 }
 
+int os::snprintf(char* buf, size_t len, const char* fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  int result = os::vsnprintf(buf, len, fmt, args);
+  va_end(args);
+  return result;
+}
+
 // Fill in buffer with current local time as an ISO-8601 string.
 // E.g., yyyy-mm-ddThh:mm:ss-zzzz.
 // Returns buffer, or NULL if it failed.

src/share/vm/runtime/os.hpp

@@ -616,6 +616,9 @@ class os: AllStatic {
   static void *find_agent_function(AgentLibrary *agent_lib, bool check_lib,
                                    const char *syms[], size_t syms_len);
 
+  static int vsnprintf(char* buf, size_t len, const char* fmt, va_list args) ATTRIBUTE_PRINTF(3, 0);
+  static int snprintf(char* buf, size_t len, const char* fmt, ...) ATTRIBUTE_PRINTF(3, 4);
+
   // Print out system information; they are called by fatal error handler.
   // Output format may be different on different platforms.
   static void print_os_info(outputStream* st);

src/share/vm/utilities/exceptions.cpp

@@ -30,6 +30,7 @@
 #include "runtime/init.hpp"
 #include "runtime/java.hpp"
 #include "runtime/javaCalls.hpp"
+#include "runtime/os.hpp"
 #include "runtime/thread.inline.hpp"
 #include "runtime/threadCritical.hpp"
 #include "utilities/events.hpp"
@@ -246,8 +247,7 @@ void Exceptions::fthrow(Thread* thread, const char* file, int line, Symbol* h_na
   va_list ap;
   va_start(ap, format);
   char msg[max_msg_size];
-  vsnprintf(msg, max_msg_size, format, ap);
-  msg[max_msg_size-1] = '\0';
+  os::vsnprintf(msg, max_msg_size, format, ap);
   va_end(ap);
   _throw_msg(thread, file, line, h_name, msg);
 }

src/share/vm/utilities/globalDefinitions_visCPP.hpp

 /*
- * Copyright (c) 1997, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -187,14 +187,6 @@ const jlong max_jlong = CONST64(0x7fffffffffffffff);
 #pragma warning( disable : 4996 ) // unsafe string functions. Same as define _CRT_SECURE_NO_WARNINGS/_CRT_SECURE_NO_DEPRICATE
 #endif
 
-inline int vsnprintf(char* buf, size_t count, const char* fmt, va_list argptr) {
-  // If number of characters written == count, Windows doesn't write a
-  // terminating NULL, so we do it ourselves.
-  int ret = _vsnprintf(buf, count, fmt, argptr);
-  if (count > 0) buf[count-1] = '\0';
-  return ret;
-}
-
 // Portability macros
 #define PRAGMA_INTERFACE
 #define PRAGMA_IMPLEMENTATION

src/share/vm/utilities/ostream.cpp

 /*
- * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -27,6 +27,7 @@
 #include "gc_implementation/shared/gcId.hpp"
 #include "oops/oop.inline.hpp"
 #include "runtime/arguments.hpp"
+#include "runtime/os.hpp"
 #include "utilities/defaultStream.hpp"
 #include "utilities/ostream.hpp"
 #include "utilities/top.hpp"
@@ -89,6 +90,8 @@ const char* outputStream::do_vsnprintf(char* buffer, size_t buflen,
                                        const char* format, va_list ap,
                                        bool add_cr,
                                        size_t& result_len) {
+  assert(buflen >= 2, "buffer too small");
+
   const char* result;
   if (add_cr)  buflen--;
   if (!strchr(format, '%')) {
@@ -101,18 +104,20 @@ const char* outputStream::do_vsnprintf(char* buffer, size_t buflen,
     result = va_arg(ap, const char*);
     result_len = strlen(result);
     if (add_cr && result_len >= buflen)  result_len = buflen-1;  // truncate
-  } else if (vsnprintf(buffer, buflen, format, ap) >= 0) {
+  } else {
+    int written = os::vsnprintf(buffer, buflen, format, ap);
+    assert(written >= 0, "vsnprintf encoding error");
     result = buffer;
-    result_len = strlen(result);
+    if ((size_t)written < buflen) {
+      result_len = written;
     } else {
       DEBUG_ONLY(warning("increase O_BUFLEN in ostream.hpp -- output truncated");)
-    result = buffer;
       result_len = buflen - 1;
-    buffer[result_len] = 0;
+    }
   }
   if (add_cr) {
     if (result != buffer) {
-      strncpy(buffer, result, buflen);
+      memcpy(buffer, result, result_len);
       result = buffer;
     }
     buffer[result_len++] = '\n';
@@ -578,6 +583,117 @@ void test_loggc_filename() {
     assert(o_result == NULL, err_msg("Too long file name after pid expansion should return NULL, but got '%s'", o_result));
   }
 }
+
+//////////////////////////////////////////////////////////////////////////////
+// Test os::vsnprintf and friends.
+
+void check_snprintf_result(int expected, size_t limit, int actual, bool expect_count) {
+  if (expect_count || ((size_t)expected < limit)) {
+    assert(expected == actual, "snprintf result not expected value");
+  } else {
+    // Make this check more permissive for jdk8u, don't assert that actual == 0.
+    // e.g. jio_vsnprintf_wrapper and jio_snprintf return -1 when expected >= limit
+    if (expected >= (int) limit) {
+      assert(actual == -1, "snprintf result should be -1 for expected >= limit");
+    } else {
+      assert(actual > 0, "snprintf result should be >0 for expected < limit");
+    }
+  }
+}
+
+// PrintFn is expected to be int (*)(char*, size_t, const char*, ...).
+// But jio_snprintf is a C-linkage function with that signature, which
+// has a different type on some platforms (like Solaris).
+template<typename PrintFn>
+void test_snprintf(PrintFn pf, bool expect_count) {
+  const char expected[] = "abcdefghijklmnopqrstuvwxyz";
+  const int expected_len = sizeof(expected) - 1;
+  const size_t padding_size = 10;
+  char buffer[2 * (sizeof(expected) + padding_size)];
+  char check_buffer[sizeof(buffer)];
+  const char check_char = '1';  // Something not in expected.
+  memset(check_buffer, check_char, sizeof(check_buffer));
+  const size_t sizes_to_test[] = {
+    sizeof(buffer) - padding_size,       // Fits, with plenty of space to spare.
+    sizeof(buffer)/2,                    // Fits, with space to spare.
+    sizeof(buffer)/4,                    // Doesn't fit.
+    sizeof(expected) + padding_size + 1, // Fits, with a little room to spare
+    sizeof(expected) + padding_size,     // Fits exactly.
+    sizeof(expected) + padding_size - 1, // Doesn't quite fit.
+    2,                                   // One char + terminating NUL.
+    1,                                   // Only space for terminating NUL.
+    0 };                                 // No space at all.
+  for (unsigned i = 0; i < ARRAY_SIZE(sizes_to_test); ++i) {
+    memset(buffer, check_char, sizeof(buffer)); // To catch stray writes.
+    size_t test_size = sizes_to_test[i];
+    ResourceMark rm;
+    stringStream s;
+    s.print("test_size: " SIZE_FORMAT, test_size);
+    size_t prefix_size = padding_size;
+    guarantee(test_size <= (sizeof(buffer) - prefix_size), "invariant");
+    size_t write_size = MIN2(sizeof(expected), test_size);
+    size_t suffix_size = sizeof(buffer) - prefix_size - write_size;
+    char* write_start = buffer + prefix_size;
+    char* write_end = write_start + write_size;
+
+    int result = pf(write_start, test_size, "%s", expected);
+
+    check_snprintf_result(expected_len, test_size, result, expect_count);
+
+    // Verify expected output.
+    if (test_size > 0) {
+      assert(0 == strncmp(write_start, expected, write_size - 1), "strncmp failure");
+      // Verify terminating NUL of output.
+      assert('\0' == write_start[write_size - 1], "null terminator failure");
+    } else {
+      guarantee(test_size == 0, "invariant");
+      guarantee(write_size == 0, "invariant");
+      guarantee(prefix_size + suffix_size == sizeof(buffer), "invariant");
+      guarantee(write_start == write_end, "invariant");
+    }
+
+    // Verify no scribbling on prefix or suffix.
+    assert(0 == strncmp(buffer, check_buffer, prefix_size), "prefix scribble");
+    assert(0 == strncmp(write_end, check_buffer, suffix_size), "suffix scribble");
+  }
+
+  // Special case of 0-length buffer with empty (except for terminator) output.
+  check_snprintf_result(0, 0, pf(NULL, 0, "%s", ""), expect_count);
+  check_snprintf_result(0, 0, pf(NULL, 0, ""), expect_count);
+}
+
+// This is probably equivalent to os::snprintf, but we're being
+// explicit about what we're testing here.
+static int vsnprintf_wrapper(char* buf, size_t len, const char* fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  int result = os::vsnprintf(buf, len, fmt, args);
+  va_end(args);
+  return result;
+}
+
+// These are declared in jvm.h; test here, with related functions.
+extern "C" {
+int jio_vsnprintf(char*, size_t, const char*, va_list);
+int jio_snprintf(char*, size_t, const char*, ...);
+}
+
+// This is probably equivalent to jio_snprintf, but we're being
+// explicit about what we're testing here.
+static int jio_vsnprintf_wrapper(char* buf, size_t len, const char* fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  int result = jio_vsnprintf(buf, len, fmt, args);
+  va_end(args);
+  return result;
+}
+
+void test_snprintf() {
+  test_snprintf(vsnprintf_wrapper, true);
+  test_snprintf(os::snprintf, true);
+  test_snprintf(jio_vsnprintf_wrapper, false); // jio_vsnprintf returns -1 on error including exceeding buffer size
+  test_snprintf(jio_snprintf, false);          // jio_snprintf calls jio_vsnprintf
+}
 #endif // PRODUCT
 
 fileStream::fileStream(const char* file_name) {

src/share/vm/utilities/ostream.hpp

@@ -258,6 +258,7 @@ class gcLogFileStream : public fileStream {
 #ifndef PRODUCT
 // unit test for checking -Xloggc:<filename> parsing result
 void test_loggc_filename();
+void test_snprintf();
 #endif
 
 void ostream_init();