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

[David Holmes]

Hi Ioi,

Thanks for taking a look at this.

On 22/05/2020 12:45 pm, Ioi Lam wrote:
> 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);

The module property story is not clean either before or after this 
change. Strictly speaking the module properties should be Unwriteable 
because they cannot be set via -D on the command-line. But that is 
actually handled as a special-case by is_internal_module_property() as 
you noted. So the old code declared them Unwriteable, protected them 
from being set by -D via a different mechanism, and then used an 
inappropriate API to actually write to them anyway (breaking the code 
for other properties). The new code marks them Writeable, so it can use 
the proper API to update them, but relies on the 
is_internal_module_property() logic to ensure they are not set by -D.

As this bug was concerned with not allowing other system properties to 
be changed when they should not be, I was not too concerned that the 
module property story remains unclean. A "proper" fix would be to 
accumulate the repeated module flags (e.g. --limit-mods=xxx) in a side 
structure (e.g. map) and then at the end of argument processing convert 
the map contents to actual Unwriteable properties. If anyone is so 
inclined they can file a RFE to do that.

> 
> 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

Sure - though I would suggest

   create_module_property
   create_numbered_module_property

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

I'll see if that is possible.

Thanks,
David

> 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
>