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

[David Holmes]

Can I get a second review please.

Thanks,
David

On 22/05/2020 3:51 pm, David Holmes wrote:
> Thanks Ioi!
> 
> David
> 
> On 22/05/2020 3:48 pm, Ioi Lam wrote:
>> Thumbs up!
>>
>> Thanks
>> - Ioi
>>
>> On 5/21/20 10:12 PM, 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
>>>>>
>>