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

[Daniel D. Daugherty]

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

src/hotspot/share/runtime/arguments.hpp
     No comments.

src/hotspot/share/runtime/arguments.cpp
     L2210:          strcmp(prop_name, "jdk.module.illegalAccess") == 0, 
"unknown module property: %s", prop_name);
     L2229:   assert(is_internal_module_property(prop_base_name), 
"unknown module property: %s", prop_base_name);
         nit - for unknown string values, I like to use '%s' so that the
         leading and trailing parts of the unknown string are obvious.
         Imagine a trailing space in the property name.

test/hotspot/jtreg/runtime/NonWriteableProperty.java
     No comments.

Thumbs up. No need to see a new webrev if you decide to tweak the
assertion messages.

Thanks for listing the testing that you did.

Dan



On 5/22/20 1:12 AM, David Holmes wrote:
> 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
>>>