8256828: ostream::print_cr() truncates buffer in copy-through case

Reviewed-by: stuefe, matsaave

src/hotspot/share/utilities/ostream.cpp

@@ -81,45 +81,67 @@ bool outputStream::update_position(const char* s, size_t len) {
 }
 
 // Execute a vsprintf, using the given buffer if necessary.
-// Return a pointer to the formatted string.
+// Return a pointer to the formatted string. Optimise for
+// strings without format specifiers, or only "%s". See
+// comments in the header file for more details.
 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--;
+  const char* result;  // The string to return. May not be the buffer.
+  size_t required_len = 0; // The length of buffer needed to avoid truncation
+                           // (excluding space for the nul terminator).
+
+  if (add_cr) { // Ensure space for CR even if truncation occurs.
+    buflen--;
+  }
+
   if (!strchr(format, '%')) {
     // constant format string
     result = format;
     result_len = strlen(result);
-    if (add_cr && result_len >= buflen)  result_len = buflen-1;  // truncate
-  } else if (format[0] == '%' && format[1] == 's' && format[2] == '\0') {
+    if (add_cr && result_len >= buflen) { // truncate
+      required_len = result_len + 1;
+      result_len = buflen - 1;
+    }
+  } else if (strncmp(format, "%s", 3) == 0) { //(format[0] == '%' && format[1] == 's' && format[2] == '\0') {
     // trivial copy-through format string
     result = va_arg(ap, const char*);
     result_len = strlen(result);
-    if (add_cr && result_len >= buflen)  result_len = buflen-1;  // truncate
+    if (add_cr && result_len >= buflen) { // truncate
+      required_len = result_len + 1;
+      result_len = buflen - 1;
+    }
   } else {
-    int required_len = os::vsnprintf(buffer, buflen, format, ap);
-    assert(required_len >= 0, "vsnprintf encoding error");
+    int required_buffer_len = os::vsnprintf(buffer, buflen, format, ap);
+    assert(required_buffer_len >= 0, "vsnprintf encoding error");
     result = buffer;
-    if ((size_t)required_len < buflen) {
+    required_len = required_buffer_len;
+    if (required_len < buflen) {
       result_len = required_len;
-    } else {
-      DEBUG_ONLY(warning("outputStream::do_vsnprintf output truncated -- buffer length is %d bytes but %d bytes are needed.",
-                         add_cr ? (int)buflen + 1 : (int)buflen, add_cr ? required_len + 2 : required_len + 1);)
+    } else { // truncation
       result_len = buflen - 1;
     }
   }
   if (add_cr) {
-    if (result != buffer) {
+    if (result != buffer) { // Need to copy to add CR
       memcpy(buffer, result, result_len);
       result = buffer;
+    } else {
+      required_len++;
     }
     buffer[result_len++] = '\n';
     buffer[result_len] = 0;
   }
+#ifdef ASSERT
+  if (required_len > result_len) {
+    warning("outputStream::do_vsnprintf output truncated -- buffer length is " SIZE_FORMAT
+            " bytes but " SIZE_FORMAT " bytes are needed.",
+            add_cr ? buflen + 1 : buflen, required_len + 1);
+  }
+#endif
   return result;
 }
 

src/hotspot/share/utilities/ostream.hpp

@@ -41,7 +41,8 @@ DEBUG_ONLY(class ResourceMark;)
 // we may use jio_printf:
 //     jio_fprintf(defaultStream::output_stream(), "Message");
 // This allows for redirection via -XX:+DisplayVMOutputToStdout and
-// -XX:+DisplayVMOutputToStderr
+// -XX:+DisplayVMOutputToStderr.
+
 class outputStream : public CHeapObjBase {
  private:
    NONCOPYABLE(outputStream);
@@ -56,6 +57,23 @@ class outputStream : public CHeapObjBase {
 
   // Returns whether a newline was seen or not
    bool update_position(const char* s, size_t len);
+
+  // Processes the given format string and the supplied arguments
+  // to produce a formatted string in the supplied buffer. Returns
+  // the formatted string (in the buffer). If the formatted string
+  // would be longer than the buffer, it is truncated.
+  //
+  // If the format string is a plain string (no format specifiers)
+  // or is exactly "%s" to print a supplied argument string, then
+  // the buffer is ignored, and we return the string directly.
+  // However, if `add_cr` is true then we have to copy the string
+  // into the buffer, which risks truncation if the string is too long.
+  //
+  // The `result_len` reference is always set to the length of the returned string.
+  //
+  // If add_cr is true then the cr will always be placed in the buffer (buffer minimum size is 2).
+  //
+  // In a debug build, if truncation occurs a VM warning is issued.
    static const char* do_vsnprintf(char* buffer, size_t buflen,
                                    const char* format, va_list ap,
                                    bool add_cr,
@@ -69,6 +87,8 @@ class outputStream : public CHeapObjBase {
    void do_vsnprintf_and_write(const char* format, va_list ap, bool add_cr) ATTRIBUTE_PRINTF(2, 0);
 
  public:
+   class TestSupport;  // Unit test support
+
    // creation
    outputStream();
    outputStream(bool has_time_stamps);
@@ -91,14 +111,18 @@ class outputStream : public CHeapObjBase {
    void set_position(int pos)   { _position = pos; }
 
    // printing
+   // Note that (v)print_cr forces the use of internal buffering to allow
+   // appending of the "cr". This can lead to truncation if the buffer is
+   // too small.
+
    void print(const char* format, ...) ATTRIBUTE_PRINTF(2, 3);
    void print_cr(const char* format, ...) ATTRIBUTE_PRINTF(2, 3);
    void vprint(const char *format, va_list argptr) ATTRIBUTE_PRINTF(2, 0);
    void vprint_cr(const char* format, va_list argptr) ATTRIBUTE_PRINTF(2, 0);
-   void print_raw(const char* str)            { write(str, strlen(str)); }
-   void print_raw(const char* str, size_t len)   { write(str,         len); }
-   void print_raw_cr(const char* str)         { write(str, strlen(str)); cr(); }
-   void print_raw_cr(const char* str, size_t len){ write(str,         len); cr(); }
+   void print_raw(const char* str) { write(str, strlen(str)); }
+   void print_raw(const char* str, size_t len) { write(str, len); }
+   void print_raw_cr(const char* str) { write(str, strlen(str)); cr(); }
+   void print_raw_cr(const char* str, size_t len){ write(str, len); cr(); }
    void print_data(void* data, size_t len, bool with_ascii, bool rel_addr=true);
    void put(char ch);
    void sp(int count = 1);