8243936: NonWriteable system properties are actually writeable

Reviewed-by: iklam, dcubed

src/hotspot/share/runtime/arguments.cpp

@@ -2144,7 +2144,7 @@ bool Arguments::check_vm_args_consistency() {
   if (status && EnableJVMCI) {
     PropertyList_unique_add(&_system_properties, "jdk.internal.vm.ci.enabled", "true",
         AddProperty, UnwriteableProperty, InternalProperty);
-    if (!create_numbered_property("jdk.module.addmods", "jdk.internal.vm.ci", addmods_count++)) {
+    if (!create_numbered_module_property("jdk.module.addmods", "jdk.internal.vm.ci", addmods_count++)) {
       return false;
     }
   }
@@ -2208,7 +2208,9 @@ bool Arguments::parse_uintx(const char* value,
   return false;
 }
 
-bool Arguments::create_property(const char* prop_name, const char* prop_value, PropertyInternal internal) {
+bool Arguments::create_module_property(const char* prop_name, const char* prop_value, PropertyInternal internal) {
+  assert(is_internal_module_property(prop_name) ||
+         strcmp(prop_name, "jdk.module.illegalAccess") == 0, "unknown module property: '%s'", prop_name);
   size_t prop_len = strlen(prop_name) + strlen(prop_value) + 2;
   char* property = AllocateHeap(prop_len, mtArguments);
   int ret = jio_snprintf(property, prop_len, "%s=%s", prop_name, prop_value);
@@ -2216,12 +2218,18 @@ bool Arguments::create_property(const char* prop_name, const char* prop_value, P
     FreeHeap(property);
     return false;
   }
-  bool added = add_property(property, UnwriteableProperty, internal);
+  // These are not strictly writeable properties as they cannot be set via -Dprop=val. But that
+  // is enforced by checking is_internal_module_property(). We need the property to be writeable so
+  // that multiple occurrences of the associated flag just causes the existing property value to be
+  // replaced ("last option wins"). Otherwise we would need to keep track of the flags and only convert
+  // to a property after we have finished flag processing.
+  bool added = add_property(property, WriteableProperty, internal);
   FreeHeap(property);
   return added;
 }
 
-bool Arguments::create_numbered_property(const char* prop_base_name, const char* prop_value, unsigned int count) {
+bool Arguments::create_numbered_module_property(const char* prop_base_name, const char* prop_value, unsigned int count) {
+  assert(is_internal_module_property(prop_base_name), "unknown module property: '%s'", prop_base_name);
   const unsigned int props_count_limit = 1000;
   const int max_digits = 3;
   const int extra_symbols_count = 3; // includes '.', '=', '\0'
@@ -2383,7 +2391,7 @@ int Arguments::process_patch_mod_option(const char* patch_mod_tail, bool* patch_
       // The path piece begins one past the module_equal sign
       add_patch_mod_prefix(module_name, module_equal + 1, patch_mod_javabase);
       FREE_C_HEAP_ARRAY(char, module_name);
-      if (!create_numbered_property("jdk.module.patch", patch_mod_tail, patch_mod_count++)) {
+      if (!create_numbered_module_property("jdk.module.patch", patch_mod_tail, patch_mod_count++)) {
         return JNI_ENOMEM;
       }
     } else {
@@ -2528,31 +2536,31 @@ jint Arguments::parse_each_vm_init_arg(const JavaVMInitArgs* args, bool* patch_m
         add_init_library(name, options);
       }
     } else if (match_option(option, "--add-reads=", &tail)) {
-      if (!create_numbered_property("jdk.module.addreads", tail, addreads_count++)) {
+      if (!create_numbered_module_property("jdk.module.addreads", tail, addreads_count++)) {
         return JNI_ENOMEM;
       }
     } else if (match_option(option, "--add-exports=", &tail)) {
-      if (!create_numbered_property("jdk.module.addexports", tail, addexports_count++)) {
+      if (!create_numbered_module_property("jdk.module.addexports", tail, addexports_count++)) {
         return JNI_ENOMEM;
       }
     } else if (match_option(option, "--add-opens=", &tail)) {
-      if (!create_numbered_property("jdk.module.addopens", tail, addopens_count++)) {
+      if (!create_numbered_module_property("jdk.module.addopens", tail, addopens_count++)) {
         return JNI_ENOMEM;
       }
     } else if (match_option(option, "--add-modules=", &tail)) {
-      if (!create_numbered_property("jdk.module.addmods", tail, addmods_count++)) {
+      if (!create_numbered_module_property("jdk.module.addmods", tail, addmods_count++)) {
         return JNI_ENOMEM;
       }
     } else if (match_option(option, "--limit-modules=", &tail)) {
-      if (!create_property("jdk.module.limitmods", tail, InternalProperty)) {
+      if (!create_module_property("jdk.module.limitmods", tail, InternalProperty)) {
         return JNI_ENOMEM;
       }
     } else if (match_option(option, "--module-path=", &tail)) {
-      if (!create_property("jdk.module.path", tail, ExternalProperty)) {
+      if (!create_module_property("jdk.module.path", tail, ExternalProperty)) {
         return JNI_ENOMEM;
       }
     } else if (match_option(option, "--upgrade-module-path=", &tail)) {
-      if (!create_property("jdk.module.upgrade.path", tail, ExternalProperty)) {
+      if (!create_module_property("jdk.module.upgrade.path", tail, ExternalProperty)) {
         return JNI_ENOMEM;
       }
     } else if (match_option(option, "--patch-module=", &tail)) {
@@ -2562,7 +2570,7 @@ jint Arguments::parse_each_vm_init_arg(const JavaVMInitArgs* args, bool* patch_m
         return res;
       }
     } else if (match_option(option, "--illegal-access=", &tail)) {
-      if (!create_property("jdk.module.illegalAccess", tail, ExternalProperty)) {
+      if (!create_module_property("jdk.module.illegalAccess", tail, ExternalProperty)) {
         return JNI_ENOMEM;
       }
     // -agentlib and -agentpath
@@ -2606,7 +2614,7 @@ jint Arguments::parse_each_vm_init_arg(const JavaVMInitArgs* args, bool* patch_m
         jio_snprintf(options, length, "%s", tail);
         add_instrument_agent("instrument", options, false);
         // java agents need module java.instrument
-        if (!create_numbered_property("jdk.module.addmods", "java.instrument", addmods_count++)) {
+        if (!create_numbered_module_property("jdk.module.addmods", "java.instrument", addmods_count++)) {
           return JNI_ENOMEM;
         }
       }
@@ -2787,7 +2795,7 @@ jint Arguments::parse_each_vm_init_arg(const JavaVMInitArgs* args, bool* patch_m
           return JNI_EINVAL;
         }
         // management agent in module jdk.management.agent
-        if (!create_numbered_property("jdk.module.addmods", "jdk.management.agent", addmods_count++)) {
+        if (!create_numbered_module_property("jdk.module.addmods", "jdk.management.agent", addmods_count++)) {
           return JNI_ENOMEM;
         }
 #else
@@ -4314,14 +4322,15 @@ void Arguments::PropertyList_unique_add(SystemProperty** plist, const char* k, c
   if (plist == NULL)
     return;
 
-  // If property key exist then update with new value.
+  // If property key exists and is writeable, then update with new value.
+  // Trying to update a non-writeable property is silently ignored.
   SystemProperty* prop;
   for (prop = *plist; prop != NULL; prop = prop->next()) {
     if (strcmp(k, prop->key()) == 0) {
       if (append == AppendProperty) {
-        prop->append_value(v);
+        prop->append_writeable_value(v);
       } else {
-        prop->set_value(v);
+        prop->set_writeable_value(v);
       }
       return;
     }

src/hotspot/share/runtime/arguments.hpp

@@ -120,6 +120,11 @@ class SystemProperty : public PathString {
     }
     return false;
   }
+  void append_writeable_value(const char *value) {
+    if (writeable()) {
+      append_value(value);
+    }
+  }
 
   // Constructor
   SystemProperty(const char* key, const char* value, bool writeable, bool internal = false);
@@ -389,8 +394,11 @@ class Arguments : AllStatic {
   static bool add_property(const char* prop, PropertyWriteable writeable=WriteableProperty,
                            PropertyInternal internal=ExternalProperty);
 
-  static bool create_property(const char* prop_name, const char* prop_value, PropertyInternal internal);
-  static bool create_numbered_property(const char* prop_base_name, const char* prop_value, unsigned int count);
+  // Used for module system related properties: converted from command-line flags.
+  // Basic properties are writeable as they operate as "last one wins" and will get overwritten.
+  // Numbered properties are never writeable, and always internal.
+  static bool create_module_property(const char* prop_name, const char* prop_value, PropertyInternal internal);
+  static bool create_numbered_module_property(const char* prop_base_name, const char* prop_value, unsigned int count);
 
   static int process_patch_mod_option(const char* patch_mod_tail, bool* patch_mod_javabase);
 

test/hotspot/jtreg/runtime/NonWriteableProperty.java

@@ -0,0 +1,40 @@
+/*
+ * Copyright (c) 2020, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+/*
+ * @test
+ * @bug 8243936
+ * @summary Ensure non-writeable system properties are not writeable
+ *
+ * @run main/othervm -Djava.vm.name=Unexpected NonWriteableProperty java.vm.name Unexpected
+ */
+
+public class NonWriteableProperty {
+    public static void main(String[] args) {
+        if (System.getProperty(args[0]).equals(args[1])) {
+            throw new RuntimeException("Non-writeable system property " +
+                                       args[0] + " was rewritten");
+        }
+    }
+}