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

[Stuart Marks]

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