Theoretical data race on java.util.logging.Handler.sealed

[Peter Levart]

On 12/18/2013 11:55 PM, Mandy Chung wrote:
>
> On 12/18/2013 9:03 AM, Peter Levart wrote:
>> Hi Mandy, Daniel,
>>
>> Here's yet another variant that reduces the doPrivileged code to just 
>> Handler's setters. This way no LogManager methods are invoked under 
>> elevated privilege:
>>
>> http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.06/ 
>>
>>
>
> This version looks good.  I like the refactoring to have the subclass 
> to call the common code Handler.configure method.  It may be better to 
> have the configure method (or a new one) that takes the default Level 
> and default Formatter instead of the package-private getters.
>
> I don't see why the handler constructors are designed to call the 
> overridden methods rather than the initialization and if a subclass 
> has its custom field, it should initialize its custom fields in its 
> constructor implementation.    Anyway this would be a separate clean 
> up task from this one.
>
> Can you also add a sanity test to verify that these handlers can be 
> constructed successfully with a security manager installed?
>

Hi Mandy, Daniel,

I didn't like the package-protected getters either. So here's another 
variant that replaces Handler.configure() method with a 
package-protected constructor which is chained from JDK subclasses:

http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/

I filed another bug that is fixed by this patch:

     https://bugs.openjdk.java.net/browse/JDK-8030801

And I created a test (see webrev.07) that almost passes when run against 
unchanged JDK 8 (the failure is just at the end when calling new 
SocketHandler(host, port) - access denied 
("java.util.logging.LoggingPermission" "control")). If I comment-out the 
System.setSecurityManager() from the test, it passes with unchanged 
code. This is to verify the test itself. When run against the patched 
JDK 8, it passes even when SecurityManager is active - this verifies two 
things:
- the behaviour of patched code is analogous to unpatched code as far as 
defaults and configured handler properties is concerned and it conforms 
to javadoc
- the patched code does not require any new permissions - it actually 
requires less, because it fixes bug 8030801.

All java/util/logging jtreg tests pass with patched code. I hope that 
"localhost" is a resolvable name on all environments and that new 
ServerSocket(0) creates a server socket bound at least to the IP address 
that "localhost" resolves to. Is this reasonable to assume?


Regards, Peter