From ca9390755bc652251bdcfd9ec2a583680a63fddf Mon Sep 17 00:00:00 2001 From: David Holmes Date: Thu, 6 Jun 2024 00:15:43 +0000 Subject: [PATCH] 8256828: ostream::print_cr() truncates buffer in copy-through case Reviewed-by: stuefe, matsaave --- src/hotspot/share/utilities/ostream.cpp | 48 +++- src/hotspot/share/utilities/ostream.hpp | 34 ++- test/hotspot/gtest/utilities/test_ostream.cpp | 254 +++++++++++++++++- 3 files changed, 317 insertions(+), 19 deletions(-) diff --git a/src/hotspot/share/utilities/ostream.cpp b/src/hotspot/share/utilities/ostream.cpp index 89335f9cf4c..858f6798e28 100644 --- a/src/hotspot/share/utilities/ostream.cpp +++ b/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; } diff --git a/src/hotspot/share/utilities/ostream.hpp b/src/hotspot/share/utilities/ostream.hpp index d39fca29ba4..98ea9f54a34 100644 --- a/src/hotspot/share/utilities/ostream.hpp +++ b/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); diff --git a/test/hotspot/gtest/utilities/test_ostream.cpp b/test/hotspot/gtest/utilities/test_ostream.cpp index 069836f2ee3..270ba59212a 100644 --- a/test/hotspot/gtest/utilities/test_ostream.cpp +++ b/test/hotspot/gtest/utilities/test_ostream.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved. * Copyright (c) 2019 SAP SE. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -27,6 +27,7 @@ #include "memory/resourceArea.hpp" #include "runtime/os.hpp" #include "utilities/globalDefinitions.hpp" +#include "utilities/defaultStream.hpp" #include "utilities/ostream.hpp" #include "unittest.hpp" @@ -123,6 +124,257 @@ TEST_VM(ostream, bufferedStream_dynamic_large) { */ +// Test helper for do_vsnprintf +class outputStream::TestSupport : AllStatic { + // Shared constants and variables for all subtests. + static const size_t buflen = 11; + static char buffer[buflen]; + static const size_t max_len = buflen - 1; + static size_t result_len; + static const char* result; + static void reset() { + result_len = 0; + result = nullptr; + buffer[0] = '\0'; + } + static const char* test(char* buf, size_t len, bool add_cr, + size_t& rlen, const char* format, ...) { + va_list ap; + va_start(ap, format); + const char* res = do_vsnprintf(buf, len, format, ap, add_cr, rlen); + va_end(ap); + return res; + } + + public: + // Case set 1: constant string with no format specifiers + static void test_constant_string() { + reset(); + // Case 1-1: no cr, no truncation, excess capacity + { + const char* str = "012345678"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, false, result_len, str); + ASSERT_EQ(result, str); + ASSERT_EQ(strlen(result), result_len); + } + reset(); + // Case 1-2: no cr, no truncation, exact capacity + { + const char* str = "0123456789"; + size_t initial_len = strlen(str); + ASSERT_EQ(initial_len, max_len); + result = test(buffer, buflen, false, result_len, str); + ASSERT_EQ(result, str); + ASSERT_EQ(strlen(result), result_len); + } + reset(); + // Case 1-3: no cr, no truncation, exceeds capacity + { + const char* str = "0123456789A"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len > max_len); + result = test(buffer, buflen, false, result_len, str); + ASSERT_EQ(result, str); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len); + } + reset(); + // Case 1-4: add cr, no truncation, excess capacity + { + const char* str = "01234567"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, true, result_len, str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len + 1); + ASSERT_TRUE(result_len <= max_len); + } + reset(); + // Case 1-5: add cr, no truncation, exact capacity + { + const char* str = "012345678"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, true, result_len, str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len + 1); + ASSERT_TRUE(result_len <= max_len); + } + reset(); + // Case 1-6: add cr, truncation + { + const char* str = "0123456789"; + size_t initial_len = strlen(str); + ASSERT_EQ(initial_len, max_len); + ::printf("Truncation warning expected: requires %d\n", (int)(initial_len + 1 + 1)); + result = test(buffer, buflen, true, result_len, str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len); + ASSERT_TRUE(result_len <= max_len); + } + } + + // Case set 2: "%s" string + static void test_percent_s_string() { + reset(); + // Case 2-1: no cr, no truncation, excess capacity + { + const char* str = "012345678"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, false, result_len, "%s", str); + ASSERT_EQ(result, str); + ASSERT_EQ(strlen(result), result_len); + } + reset(); + // Case 2-2: no cr, no truncation, exact capacity + { + const char* str = "0123456789"; + size_t initial_len = strlen(str); + ASSERT_EQ(initial_len, max_len); + result = test(buffer, buflen, false, result_len, "%s", str); + ASSERT_EQ(result, str); + ASSERT_EQ(strlen(result), result_len); + } + reset(); + // Case 2-3: no cr, no truncation, exceeds capacity + { + const char* str = "0123456789A"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len > max_len); + result = test(buffer, buflen, false, result_len, "%s", str); + ASSERT_EQ(result, str); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len); + } + reset(); + // Case 2-4: add cr, no truncation, excess capacity + { + const char* str = "01234567"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, true, result_len, "%s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len + 1); + ASSERT_TRUE(result_len <= max_len); + } + reset(); + // Case 2-5: add cr, no truncation, exact capacity + { + const char* str = "012345678"; + size_t initial_len = strlen(str); + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, true, result_len, "%s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len + 1); + ASSERT_TRUE(result_len <= max_len); + } + reset(); + // Case 2-6: add cr, truncation + { + const char* str = "0123456789"; + size_t initial_len = strlen(str); + ASSERT_EQ(initial_len, max_len); + ::printf("Truncation warning expected: requires %d\n", (int)(initial_len + 1 + 1)); + result = test(buffer, buflen, true, result_len, "%s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len); + ASSERT_TRUE(result_len <= max_len); + } + } + + // Case set 3: " %s" string - the space means we avoid the pass-through optimization and use vsnprintf + static void test_general_string() { + reset(); + // Case 3-1: no cr, no truncation, excess capacity + { + const char* str = "01234567"; + size_t initial_len = strlen(str) + 1; + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, false, result_len, " %s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + } + reset(); + // Case 3-2: no cr, no truncation, exact capacity + { + const char* str = "012345678"; + size_t initial_len = strlen(str) + 1; + ASSERT_EQ(initial_len, max_len); + result = test(buffer, buflen, false, result_len, " %s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + } + reset(); + // Case 3-3: no cr, truncation + { + const char* str = "0123456789"; + size_t initial_len = strlen(str) + 1; + ASSERT_TRUE(initial_len > max_len); + ::printf("Truncation warning expected: requires %d\n", (int)(initial_len + 1)); + result = test(buffer, buflen, false, result_len, " %s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + } + reset(); + // Case 3-4: add cr, no truncation, excess capacity + { + const char* str = "0123456"; + size_t initial_len = strlen(str) + 1; + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, true, result_len, " %s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len + 1); + ASSERT_TRUE(result_len <= max_len); + } + reset(); + // Case 3-5: add cr, no truncation, exact capacity + { + const char* str = "01234567"; + size_t initial_len = strlen(str) + 1; + ASSERT_TRUE(initial_len < max_len); + result = test(buffer, buflen, true, result_len, " %s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len + 1); + ASSERT_TRUE(result_len <= max_len); + } + reset(); + // Case 3-6: add cr, truncation + { + const char* str = "012345678"; + size_t initial_len = strlen(str) + 1; + ASSERT_EQ(initial_len, max_len); + ::printf("Truncation warning expected: requires %d\n", (int)(initial_len + 1 + 1)); + result = test(buffer, buflen, true, result_len, " %s", str); + ASSERT_EQ(result, buffer); + ASSERT_EQ(strlen(result), result_len); + ASSERT_EQ(result_len, initial_len); + ASSERT_TRUE(result_len <= max_len); + } + } + +}; + +const size_t outputStream::TestSupport::max_len; +char outputStream::TestSupport::buffer[outputStream::TestSupport::buflen]; +size_t outputStream::TestSupport::result_len = 0; +const char* outputStream::TestSupport::result = nullptr; + +TEST_VM(ostream, do_vsnprintf_buffering) { + outputStream::TestSupport::test_constant_string(); + outputStream::TestSupport::test_percent_s_string(); + outputStream::TestSupport::test_general_string(); +}