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

[Daniel Fuchs]

Hi Peter,

Good idea to add a package constructor in Handler.
It looks much cleaner than the configure method.

Good tests too - thanks for adding that!

To access files with JPRT there is a simpler manner (though what
you have looks good).
JPRT sets a system property "test.src" which point at the
directory where the sources are located.

So you could have used something like:

System.setProperty(CONFIG_FILE_PROPERTY,
    new File(System.getProperty("test.src", "."),
             this.getClass().getSimpleName()+".props")
             .getAbsolutePath());

best regards,

-- daniel

On 12/19/13 4:49 PM, Peter Levart wrote:
> 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
>