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

[David Holmes]

On 22/05/2020 1:04 pm, David Holmes wrote:
> 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.

It was possible with a special escape clause for 
jdk.module.illegalAccess which for some reason is not included in the 
set of internal module properties. That may be an oversight but it isn't 
something I can change.

Updated some commentary too.

Updated webrev:

http://cr.openjdk.java.net/~dholmes/8243936/webrev.v2/

Thanks,
David

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