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

[David Holmes]

Hi Dan,

Thanks for looking at this. I will make the '%s' adjustment.

Thanks,
David

On 28/05/2020 12:44 am, Daniel D. Daugherty wrote:
>> 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
>>>>
>