RFR 9+-: 8180582: The bind to rmiregistry is rejected by registryFilter even though registryFilter is set

[Stuart Marks]

Hi Roger,

Ah, so your alternative to fixing up the regex is to avoid using regexes 
entirely! :-)

Seriously, removing the regex is an improvement; it reduces a major source of 
fragility.

The update looks fine. I did notice one typo:

  82      * Data RMI Regiry bind test.

No need for another webrev for the fix for this one.

Thanks,

s'marks


On 5/31/17 7:09 PM, Roger Riggs wrote:
> Hi Stuart,
>
> Good suggestions on the regex, but it will be simpler, as you suggest, to use a
> separate
> command line property.
>
> The webrev is updated in place.
> http://cr.openjdk.java.net/~rriggs/webrev-depth-max-8180582/index.html
>
> Thanks, Roger
>
>
>
> On 5/31/17 7:09 PM, Stuart Marks wrote:
>> Hi Roger,
>>
>> RegistryImpl change looks fine. I have a quibble on the regex used in the test.
>>
>>  162         Matcher m = Pattern
>>  163                 .compile(".*maxdepth=(\\d*)")
>>  164 .matcher(Objects.requireNonNullElse(registryFilter, ""));
>>  165         int depthOverride = m.find() ? Integer.parseInt(m.group(1)) :
>> REGISTRY_MAX_DEPTH;
>>
>> Since Matcher.find() is called, the pattern can be found anywhere in the input
>> string, so the leading ".*" is unnecessary. I'd suggest replacing it with
>> "\\b" to match a word boundary immediately preceding "maxdepth". Also, "\\d*"
>> will match zero characters, which could cause the subsequent parseInt() to fail.
>>
>> These issues aren't of immediate concern, as the test works as it stands. They
>> may make the test a bit harder to maintain, though.
>>
>> But I think the main concern is that if the regex were modified by future
>> maintenance, it might not match, which would cause the test to test
>> REGISTRY_MAX_DEPTH redundantly instead of the depth the filter is attempting
>> to specify. This problem wouldn't be completely silent, but it'd be easy to
>> miss. You'd have to inspect the log pretty carefully to see what's going on.
>> So maybe it would be good to have something like a command-line argument that
>> specifies the expected value of maxdepth. Yes, this is redundant, but it's a
>> useful cross-check, I think.
>>
>> There are several identical lines of code starting at line 184. Maybe these
>> could be extracted into a common method.
>>
>> These are all cleanup issues, so it's up to you how much additional work you
>> want to put into this before it gets into 9.
>>
>> s'marks
>>
>>
>>
>> On 5/31/17 7:02 AM, Roger Riggs wrote:
>>> Thanks Daniel for the review and corrected link.
>>>
>>> On 5/30/2017 7:04 PM, Daniel Fuchs wrote:
>>>> Hi Roger,
>>>>
>>>> On 30/05/17 19:26, Roger Riggs wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> Fixed, there is no need for the unbind since the registry is not reused and
>>>>> it might
>>>>> cause a spurious success.
>>>>>
>>>>> (Also corrected the exception error message @ 151 and 153).
>>>>>
>>>>> Webrev updated in place.
>>>>> ...
>>>>
>>>> I assume you meant
>>>> http://cr.openjdk.java.net/~rriggs/webrev-depth-max-8180582/index.html
>>>> which is what I reviewed ;-)
>>>>
>>>> Looks good!
>>>>
>>>> -- danel
>>>>
>>>>>
>>>>> Thanks, Roger
>>>>>
>>>>>
>>>>> On 5/30/2017 2:00 PM, Daniel Fuchs wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> Looks good! Just one nit with the test:
>>>>>>
>>>>>>  194             registry.unbind(name);
>>>>>>  195             Assert.fail("Registry filter should have rejected depth: "
>>>>>> + depthOverride + 1);
>>>>>>
>>>>>> Technically, the test will also pass if bind() succeed but
>>>>>> unbind() fails - for some unforeseen reason.
>>>>>>
>>>>>> best regards,
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>> On 25/05/17 16:31, Roger Riggs wrote:
>>>>>>> Please review an update to the RMI Registry built-in serial filter
>>>>>>> implementation limit on the depth
>>>>>>> of a instance that can be bound in the registry.  The initial depth limit
>>>>>>> was 5 and it was too limiting
>>>>>>> for some existing applications.  It limit is updated to depth = 20 and
>>>>>>> should be more than adequate.
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-depth-max-8180582/index.html
>>>>>>>
>>>>>>> Issue:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8180582
>>>>>>>
>>>>>>> Thanks, Roger
>>>>>>
>>>>>
>>>>
>>>
>