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

[Daniel Fuchs]

On 12/16/13 10:14 PM, Mandy Chung wrote:
> On 12/14/2013 9:38 AM, Peter Levart wrote:
>> Hi,
>>
>> Daniel reminded me of a couple of issues the 4th revision of the patch
>> would have when backporting to 7u. So here's another variant that
>> tries to be more backport-friendly:
>>
>> http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.05/
>
> This looks good in general.
>
> SocketHandler line 200 - it looks to me that this is an existing bug
> that should call setOutputStream within doPrivileged.

I'm not sure I would qualify this as a bug: if you want to create a
SocketHandler that sends to a specific host/port you need the
LoggingPermission("control").

> I think it'd be simpler if SocketHandler no-arg constructor can first
> get the port and host from the logging properties so that it doesn't
> need to differentiate hostAndPortSet is set and ConfigureAction no-arg
> constructor can be removed.
>
> Daniel/Peter - do we have tests to cover these permission check for
> these handlers?

Shockingly:

$ cd jdk/test/java/util/logging
$ find . -type f -exec grep SocketHandler {} /dev/null \;

reveals nothing. So it seams we have no unit tests for SocketHandler!

-- daniel

>
> Mandy
>
>>
>> This variant could be backported by simply replacing a limited variant
>> of doPrivileged (introduced in JDK 8) with full variant and still not
>> elevate the privilege of Socket creation in SocketHandler. I also
>> removed the need to subclass various ConfigureAction(s) with
>> annonymous inner subclasses by introducing overloaded constructors to
>> ConfigureActions(s) that follow the overloaded constructors of various
>> Handlers.
>>
>> Regards, Peter
>>
>> On 12/14/2013 12:25 PM, Peter Levart wrote:
>>> Hi Mandy,
>>>
>>> On 12/13/2013 12:37 AM, Mandy Chung wrote:
>>>> Hi Peter,
>>>>
>>>> On 12/8/2013 11:19 AM, Peter Levart wrote:
>>>>> H Mandy,
>>>>>
>>>>> I created an issue for it nevertheless:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8029781
>>>>>
>>>>> You're right, doPrivileged() is a more straight-forward approach
>>>>> than 'sealed' variable. Since this might only be considered for
>>>>> inclusion in JDK9 when lambdas are already a tried technology, how
>>>>> do you feel about using them for platform code like logging? As far
>>>>> as I know (just checked), lambda meta-factory is not using any
>>>>> j.u.logging, so there is no danger of initialization loops or similar:
>>>>>
>>>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/
>>>>>
>>>>
>>>> Sorry for the delay to get to this.
>>>
>>> No rush. We have time before JDK9 gets set-up and running...
>>>
>>>>
>>>> Alan is right that java.lang.invoke.ProxyClassesDumper does use
>>>> PlatformLogger which will forward calls to j.u.logging if
>>>> -Djava.util.logging.config.file is set or java.util.logging has been
>>>> initialized (via other j.u.logging call).  It means that it may lead
>>>> to recursive initialization.  Also the current PlatformLogger
>>>> implementation formats the message in the same way as j.u.logging
>>>> that may load locale providers and other classes.  I am afraid there
>>>> are issues to tease out and resolve.
>>>
>>> It's unfortunate that a lambda debugging feature prevents us from
>>> using a basic language feature in j.u.logging code. As far as I know,
>>> java.lang.invoke.ProxyClassesDumper is only used if
>>> 'jdk.internal.lambda.dumpProxyClasses' system property is set to
>>> point to a directory where lambda proxy class files are to be dumped
>>> as they are generated - a debugging hook therefore. Wouldn't it be
>>> good-enough if error messages about not-being able to set-up/use the
>>> dump facility were output to System.err directly - not using
>>> PlatformLogger at all?
>>>
>>>>
>>>> The overloads the doPrivileged method makes it cumbersome to use
>>>> lambda that causes you to workaround it by adding a new
>>>> PrivilegedVoidAction interface which is clever.  So I think it isn't
>>>> too bad for this patch to use anonymous inner class and have the
>>>> doPrivileged call in the constructor.
>>>
>>> Right. I have prepared a modified webrev which doesn't use lambdas:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.04/
>>>
>>> In attempt to minimize the boilerplate, I haven't just replaced
>>> lambdas with anonymous inner classes, but rather turned all private
>>> configure() methods into ConfigureAction inner classes. In two
>>> occasions (SocketHandler and StreamHandler), they are extended with
>>> anonymous inner classes to append some actions. In SocketHandler I
>>> kept the mechanics of transporting the checked IOException out of
>>> PrivilegedAction by wrapping it with Unchecked IOException and later
>>> unwrapping and throwing it, rather than using
>>> PrivilegedExceptionAction which would further complicate exception
>>> handling, since it declares throwing a general j.l.Exception, but the
>>> SocketHandler constructor only declares throwing IOException...
>>>
>>> I think this could be backported to 7u as-is.
>>>
>>> Regards, Peter
>>>
>>>>
>>>>
>>>> Mandy
>>>
>>
>