RFR (S): 8243936: NonWriteable system properties are actually writeable

[Ioi Lam]

Hi David,

This change reasonable to me. It took me a while to understand the fix 
and the history behind it. I can see that the module properties can be 
"writable" because they are checked in this function to prevent them 
from being set via -D in the command-line.

bool Arguments::is_internal_module_property(const char* property) {
   assert((strncmp(property, "-D", 2) != 0), "Unexpected leading -D");
   if  (strncmp(property, MODULE_PROPERTY_PREFIX, 
MODULE_PROPERTY_PREFIX_LEN) == 0) {
     const char* property_suffix = property + MODULE_PROPERTY_PREFIX_LEN;
     if (matches_property_suffix(property_suffix, ADDEXPORTS, 
ADDEXPORTS_LEN) ||
         matches_property_suffix(property_suffix, ADDREADS, ADDREADS_LEN) ||
         matches_property_suffix(property_suffix, ADDOPENS, ADDOPENS_LEN) ||
         matches_property_suffix(property_suffix, PATCH, PATCH_LEN) ||
         matches_property_suffix(property_suffix, ADDMODS, ADDMODS_LEN) ||
         matches_property_suffix(property_suffix, LIMITMODS, 
LIMITMODS_LEN) ||
         matches_property_suffix(property_suffix, PATH, PATH_LEN) ||
         matches_property_suffix(property_suffix, UPGRADE_PATH, 
UPGRADE_PATH_LEN)) {
       return true;
     }
   }
   return false;
}

... which is why this line no longer needs to say that a module property 
is "UnwriteableProperty"

-  bool added = add_property(property, UnwriteableProperty, internal);
+  bool added = add_property(property, WriteableProperty, internal);


I have some nits:


  397   // Used for module system related properties: converted from 
command-line flags
  398   // Basic properties are writeable as they operate as "last one 
wins" and will get overwritten.
  399   // Numbered properties are never writeable, and always internal.
  400   static bool create_property(const char* prop_name, const char* 
prop_value, PropertyInternal internal);
  401   static bool create_numbered_property(const char* prop_base_name, 
const char* prop_value, unsigned int count);

I am not sure if anyone will read these comments and these functions may 
be misused in the future. How about renaming them to

     create_property_for_module
     create_numbered_property_for_module

BTW, is it possible to assert for is_internal_module_property() in these 
two functions?

Thanks
- Ioi


On 5/21/20 3:21 PM, David Holmes wrote:
> webrev: http://cr.openjdk.java.net/~dholmes/8243936/webrev/
> bug: https://bugs.openjdk.java.net/browse/JDK-8243936
>
> Please see details in the bug report.
>
> tld;dr: we again call the set method that checks whether a flag is 
> writeable, and I added the equivalent append version. Added some 
> commentary about the methods used only for module options and changed 
> the basic module properties to be writeable as they need to allow 
> overriding.
>
> Testing:
>   - added a new test that verifies you can't set a non-writeable 
> property. (There's no test for the append case because we don't have 
> any appendable non-writeable flags)
>  - ran all the module related tests I could find to ensure no impact 
> on the module flags
>   - jdk/tools/launcher/modules/
>   - jdk/com/sun/tools/attach/modules
>   - jdk/java/lang/Class/forName/modules
>   - jdk/java/lang/ClassLoader/getResource/modules
>   - jdk/java/lang/SecurityManager/modules
>   - jdk/java/lang/System/LoggerFinder/modules
>   - jdk/java/lang/instrument/modules
>   - jdk/java/lang/invoke/modules
>   - jdk/java/sql/modules
>   - jdk/java/util/ResourceBundle/modules
>   - jdk/java/util/ServiceLoader/modules
>   - jdk/java/util/logging/modules
>   - jdk/javax/security/auth/login/modules
>   - jdk/jdk/jfr/api/modules
>   - jdk/jdk/modules
>   - hotspot/jtreg/runtime/modules/
>  - tiers 1-3
>
> Thanks,
> David