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

Peter Levart peter.levart at gmail.com
Tue Dec 17 16:22:51 UTC 2013


On 12/17/2013 11:29 AM, Daniel Fuchs wrote:
> 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 was asking myself the same question.

>
> 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").

What is being protected with permission checking in Handler(s) is 
changing various properties of Handler(s). What we are granting with 
doPriviliged is a temporary elevation of privilege for changing the 
properties of the instance of Handler that is being constructed - just 
for the act of constructing the instance of Handler. Anybody should be 
able to instantiate new Handler instance - that doesn't present any 
special capability. Putting new Handler into service is already 
additionally protected (for example Logger.addHandler()). So in case of 
SocketHandler, one should be able to instantiate new SocketHandler for 
specified host/port if she has permission to create a Socket to that 
host/port, but nothing more should be required. To put such Handler into 
service (to direct logging messages generated by logging system to it), 
additional LoggingPermission("control") is already required.

So I think Mandy is right and that is an existing bug, although not very 
critical, since it only affects those that want to instantiate new 
SocketHandler although they are not able to use it in the logging system 
(maybe they use it somewhere else?).

Regards, Peter

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




More information about the core-libs-dev mailing list