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

[Roger Riggs]

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