8178491: -Xss and -XX:ThreadStackSize argument parsing truncates bits

Reviewed-by: gziemski, kbarrett

hotspot/src/share/vm/runtime/arguments.cpp

@@ -644,10 +644,9 @@ bool Arguments::atojulong(const char *s, julong* result) {
   }
 }
 
-Arguments::ArgsRange Arguments::check_memory_size(julong size, julong min_size) {
+Arguments::ArgsRange Arguments::check_memory_size(julong size, julong min_size, julong max_size) {
   if (size < min_size) return arg_too_small;
-  // Check that size will fit in a size_t (only relevant on 32-bit)
+  if (size > max_size) return arg_too_big;
-  if (size > max_uintx) return arg_too_big;
   return arg_in_range;
 }
 
@@ -2617,9 +2616,10 @@ bool Arguments::create_numbered_property(const char* prop_base_name, const char*
 
 Arguments::ArgsRange Arguments::parse_memory_size(const char* s,
                                                   julong* long_arg,
-                                                  julong min_size) {
+                                                  julong min_size,
+                                                  julong max_size) {
   if (!atojulong(s, long_arg)) return arg_unreadable;
-  return check_memory_size(*long_arg, min_size);
+  return check_memory_size(*long_arg, min_size, max_size);
 }
 
 // Parse JavaVMInitArgs structure
@@ -2750,6 +2750,55 @@ int Arguments::process_patch_mod_option(const char* patch_mod_tail, bool* patch_
   return JNI_OK;
 }
 
+// Parse -Xss memory string parameter and convert to ThreadStackSize in K.
+jint Arguments::parse_xss(const JavaVMOption* option, const char* tail, intx* out_ThreadStackSize) {
+  // The min and max sizes match the values in globals.hpp, but scaled
+  // with K. The values have been chosen so that alignment with page
+  // size doesn't change the max value, which makes the conversions
+  // back and forth between Xss value and ThreadStackSize value easier.
+  // The values have also been chosen to fit inside a 32-bit signed type.
+  const julong min_ThreadStackSize = 0;
+  const julong max_ThreadStackSize = 1 * M;
+
+  const julong min_size = min_ThreadStackSize * K;
+  const julong max_size = max_ThreadStackSize * K;
+
+  assert(is_size_aligned_(max_size, (size_t)os::vm_page_size()), "Implementation assumption");
+
+  julong size = 0;
+  ArgsRange errcode = parse_memory_size(tail, &size, min_size, max_size);
+  if (errcode != arg_in_range) {
+    bool silent = (option == NULL); // Allow testing to silence error messages
+    if (!silent) {
+      jio_fprintf(defaultStream::error_stream(),
+                  "Invalid thread stack size: %s\n", option->optionString);
+      describe_range_error(errcode);
+    }
+    return JNI_EINVAL;
+  }
+
+  // Internally track ThreadStackSize in units of 1024 bytes.
+  const julong size_aligned = align_size_up_(size, K);
+  assert(size <= size_aligned,
+         "Overflow: " JULONG_FORMAT " " JULONG_FORMAT,
+         size, size_aligned);
+
+  const julong size_in_K = size_aligned / K;
+  assert(size_in_K < (julong)max_intx,
+         "size_in_K doesn't fit in the type of ThreadStackSize: " JULONG_FORMAT,
+         size_in_K);
+
+  // Check that code expanding ThreadStackSize to a page aligned number of bytes won't overflow.
+  const julong max_expanded = align_size_up_(size_in_K * K, (size_t)os::vm_page_size());
+  assert(max_expanded < max_uintx && max_expanded >= size_in_K,
+         "Expansion overflowed: " JULONG_FORMAT " " JULONG_FORMAT,
+         max_expanded, size_in_K);
+
+  *out_ThreadStackSize = (intx)size_in_K;
+
+  return JNI_OK;
+}
+
 jint Arguments::parse_each_vm_init_arg(const JavaVMInitArgs* args, bool* patch_mod_javabase, Flag::Flags origin) {
   // For match_option to return remaining or value part of option string
   const char* tail;
@@ -3013,17 +3062,12 @@ jint Arguments::parse_each_vm_init_arg(const JavaVMInitArgs* args, bool* patch_m
       }
     // -Xss
     } else if (match_option(option, "-Xss", &tail)) {
-      julong long_ThreadStackSize = 0;
+      intx value = 0;
-      ArgsRange errcode = parse_memory_size(tail, &long_ThreadStackSize, 1000);
+      jint err = parse_xss(option, tail, &value);
-      if (errcode != arg_in_range) {
+      if (err != JNI_OK) {
-        jio_fprintf(defaultStream::error_stream(),
+        return err;
-                    "Invalid thread stack size: %s\n", option->optionString);
-        describe_range_error(errcode);
-        return JNI_EINVAL;
       }
-      // Internally track ThreadStackSize in units of 1024 bytes.
+      if (FLAG_SET_CMDLINE(intx, ThreadStackSize, value) != Flag::SUCCESS) {
-      if (FLAG_SET_CMDLINE(intx, ThreadStackSize,
-                       round_to((int)long_ThreadStackSize, K) / K) != Flag::SUCCESS) {
         return JNI_EINVAL;
       }
     // -Xoss, -Xsqnopause, -Xoptimize, -Xboundthreads, -Xusealtsigs

hotspot/src/share/vm/runtime/arguments.hpp

@@ -322,6 +322,7 @@ class Arguments : AllStatic {
   friend class VMStructs;
   friend class JvmtiExport;
   friend class CodeCacheExtensions;
+  friend class ArgumentsTest;
  public:
   // Operation modi
   enum Mode {
@@ -514,6 +515,7 @@ class Arguments : AllStatic {
   static jint parse_java_options_environment_variable(ScopedVMInitArgs* vm_args);
   static jint parse_vm_options_file(const char* file_name, ScopedVMInitArgs* vm_args);
   static jint parse_options_buffer(const char* name, char* buffer, const size_t buf_len, ScopedVMInitArgs* vm_args);
+  static jint parse_xss(const JavaVMOption* option, const char* tail, intx* out_ThreadStackSize);
   static jint insert_vm_options_file(const JavaVMInitArgs* args,
                                      const char* vm_options_file,
                                      const int vm_options_file_pos,
@@ -542,9 +544,9 @@ class Arguments : AllStatic {
   }
 
   static void describe_range_error(ArgsRange errcode);
-  static ArgsRange check_memory_size(julong size, julong min_size);
+  static ArgsRange check_memory_size(julong size, julong min_size, julong max_size);
   static ArgsRange parse_memory_size(const char* s, julong* long_arg,
-                                     julong min_size);
+                                     julong min_size, julong max_size = max_uintx);
   // Parse a string for a unsigned integer.  Returns true if value
   // is an unsigned integer greater than or equal to the minimum
   // parameter passed and returns the value in uintx_arg.  Returns

hotspot/src/share/vm/runtime/globals.hpp

@@ -3295,7 +3295,7 @@ public:
                                                                             \
   product_pd(intx, ThreadStackSize,                                         \
           "Thread Stack Size (in Kbytes)")                                  \
-          range(0, (max_intx-os::vm_page_size())/(1 * K))                   \
+          range(0, 1 * M)                                                   \
                                                                             \
   product_pd(intx, VMThreadStackSize,                                       \
           "Non-Java Thread Stack Size (in Kbytes)")                         \

hotspot/test/native/runtime/test_arguments.cpp

@@ -27,7 +27,22 @@
 #include "unittest.hpp"
 #include "utilities/globalDefinitions.hpp"
 
-TEST(arguments, atojulong) {
+class ArgumentsTest : public ::testing::Test {
+public:
+  static intx parse_xss_inner_annotated(const char* str, jint expected_err, const char* file, int line_number);
+
+  // Expose the private Arguments functions.
+
+  static Arguments::ArgsRange check_memory_size(julong size, julong min_size, julong max_size) {
+    return Arguments::check_memory_size(size, min_size, max_size);
+  }
+
+  static jint parse_xss(const JavaVMOption* option, const char* tail, intx* out_ThreadStackSize) {
+    return Arguments::parse_xss(option, tail, out_ThreadStackSize);
+  }
+};
+
+TEST_F(ArgumentsTest, atojulong) {
   char ullong_max[32];
   int ret = jio_snprintf(ullong_max, sizeof(ullong_max), JULONG_FORMAT, ULLONG_MAX);
   ASSERT_NE(-1, ret);
@@ -70,3 +85,118 @@ TEST(arguments, atojulong) {
     ASSERT_EQ(valid_strings[i].expected_value, value);
   }
 }
+
+TEST_F(ArgumentsTest, check_memory_size__min) {
+  EXPECT_EQ(check_memory_size(999,  1000, max_uintx), Arguments::arg_too_small);
+  EXPECT_EQ(check_memory_size(1000, 1000, max_uintx), Arguments::arg_in_range);
+  EXPECT_EQ(check_memory_size(1001, 1000, max_uintx), Arguments::arg_in_range);
+
+  EXPECT_EQ(check_memory_size(max_intx - 2, max_intx - 1, max_uintx), Arguments::arg_too_small);
+  EXPECT_EQ(check_memory_size(max_intx - 1, max_intx - 1, max_uintx), Arguments::arg_in_range);
+  EXPECT_EQ(check_memory_size(max_intx - 0, max_intx - 1, max_uintx), Arguments::arg_in_range);
+
+  EXPECT_EQ(check_memory_size(max_intx - 1, max_intx, max_uintx), Arguments::arg_too_small);
+  EXPECT_EQ(check_memory_size(max_intx    , max_intx, max_uintx), Arguments::arg_in_range);
+
+  NOT_LP64(
+    EXPECT_EQ(check_memory_size((julong)max_intx + 1, max_intx, max_uintx), Arguments::arg_in_range);
+
+    EXPECT_EQ(check_memory_size(        max_intx - 1, (julong)max_intx + 1, max_uintx), Arguments::arg_too_small);
+    EXPECT_EQ(check_memory_size(        max_intx    , (julong)max_intx + 1, max_uintx), Arguments::arg_too_small);
+    EXPECT_EQ(check_memory_size((julong)max_intx + 1, (julong)max_intx + 1, max_uintx), Arguments::arg_in_range);
+    EXPECT_EQ(check_memory_size((julong)max_intx + 2, (julong)max_intx + 1, max_uintx), Arguments::arg_in_range);
+  );
+
+  EXPECT_EQ(check_memory_size(max_uintx - 2, max_uintx - 1, max_uintx), Arguments::arg_too_small);
+  EXPECT_EQ(check_memory_size(max_uintx - 1, max_uintx - 1, max_uintx), Arguments::arg_in_range);
+  EXPECT_EQ(check_memory_size(max_uintx    , max_uintx - 1, max_uintx), Arguments::arg_in_range);
+
+  EXPECT_EQ(check_memory_size(max_uintx - 1, max_uintx, max_uintx), Arguments::arg_too_small);
+  EXPECT_EQ(check_memory_size(max_uintx    , max_uintx, max_uintx), Arguments::arg_in_range);
+}
+
+TEST_F(ArgumentsTest, check_memory_size__max) {
+  EXPECT_EQ(check_memory_size(max_uintx - 1, 1000, max_uintx), Arguments::arg_in_range);
+  EXPECT_EQ(check_memory_size(max_uintx    , 1000, max_uintx), Arguments::arg_in_range);
+
+  EXPECT_EQ(check_memory_size(max_intx - 2     , 1000, max_intx - 1), Arguments::arg_in_range);
+  EXPECT_EQ(check_memory_size(max_intx - 1     , 1000, max_intx - 1), Arguments::arg_in_range);
+  EXPECT_EQ(check_memory_size(max_intx         , 1000, max_intx - 1), Arguments::arg_too_big);
+
+  EXPECT_EQ(check_memory_size(max_intx - 1     , 1000, max_intx), Arguments::arg_in_range);
+  EXPECT_EQ(check_memory_size(max_intx         , 1000, max_intx), Arguments::arg_in_range);
+
+  NOT_LP64(
+    EXPECT_EQ(check_memory_size((julong)max_intx + 1     , 1000, max_intx), Arguments::arg_too_big);
+
+    EXPECT_EQ(check_memory_size(        max_intx         , 1000, (julong)max_intx + 1), Arguments::arg_in_range);
+    EXPECT_EQ(check_memory_size((julong)max_intx + 1     , 1000, (julong)max_intx + 1), Arguments::arg_in_range);
+    EXPECT_EQ(check_memory_size((julong)max_intx + 2     , 1000, (julong)max_intx + 1), Arguments::arg_too_big);
+ );
+}
+
+// A random value - used to verify the output when parsing is expected to fail.
+static const intx no_value = 4711;
+
+inline intx ArgumentsTest::parse_xss_inner_annotated(const char* str, jint expected_err, const char* file, int line_number) {
+  intx value = no_value;
+  jint err = parse_xss(NULL /* Silence error messages */, str, &value);
+  EXPECT_EQ(err, expected_err) << "Failure from: " << file << ":" << line_number;
+  return value;
+}
+
+// Wrapper around the help function - gives file and line number when a test failure occurs.
+#define parse_xss_inner(str, expected_err) ArgumentsTest::parse_xss_inner_annotated(str, expected_err, __FILE__, __LINE__)
+
+static intx calc_expected(julong small_xss_input) {
+  assert(small_xss_input <= max_julong / 2, "Sanity");
+
+  // Match code in arguments.cpp
+  julong julong_ret = align_size_up_(small_xss_input, K) / K;
+  assert(julong_ret <= (julong)max_intx, "Overflow: " JULONG_FORMAT, julong_ret);
+  return (intx)julong_ret;
+}
+
+static char buff[100];
+static char* to_string(julong value) {
+  jio_snprintf(buff, sizeof(buff), JULONG_FORMAT, value);
+  return buff;
+}
+
+TEST_VM_F(ArgumentsTest, parse_xss) {
+  // Test the maximum input value - should fail.
+  {
+    EXPECT_EQ(parse_xss_inner(to_string(max_julong), JNI_EINVAL), no_value);
+    NOT_LP64(EXPECT_EQ(parse_xss_inner(to_string(max_uintx), JNI_EINVAL), no_value));
+  }
+
+  // Test values "far" away from the uintx boundary,
+  // but still beyond the max limit.
+  {
+    LP64_ONLY(EXPECT_EQ(parse_xss_inner(to_string(max_julong / 2), JNI_EINVAL), no_value));
+    EXPECT_EQ(parse_xss_inner(to_string(INT_MAX),     JNI_EINVAL), no_value);
+  }
+
+  // Test at and around the max limit.
+  {
+    EXPECT_EQ(parse_xss_inner(to_string(1 * M * K - 1), JNI_OK), calc_expected(1 * M * K - 1));
+    EXPECT_EQ(parse_xss_inner(to_string(1 * M * K),     JNI_OK), calc_expected(1 * M * K));
+    EXPECT_EQ(parse_xss_inner(to_string(1 * M * K + 1), JNI_EINVAL), no_value);
+  }
+
+  // Test value aligned both to K and vm_page_size.
+  {
+    EXPECT_TRUE(is_size_aligned(32 * M, K));
+    EXPECT_TRUE(is_size_aligned(32 * M, (size_t)os::vm_page_size()));
+    EXPECT_EQ(parse_xss_inner(to_string(32 * M), JNI_OK), (intx)(32 * M / K));
+  }
+
+  // Test around the min limit.
+  {
+    EXPECT_EQ(parse_xss_inner(to_string(0),     JNI_OK), calc_expected(0));
+    EXPECT_EQ(parse_xss_inner(to_string(1),     JNI_OK), calc_expected(1));
+    EXPECT_EQ(parse_xss_inner(to_string(K - 1), JNI_OK), calc_expected(K - 1));
+    EXPECT_EQ(parse_xss_inner(to_string(K),     JNI_OK), calc_expected(K));
+    EXPECT_EQ(parse_xss_inner(to_string(K + 1), JNI_OK), calc_expected(K + 1));
+  }
+}