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

Wed Dec 18 11:05:57 UTC 2013

On 12/18/13 12:55 AM, Peter Levart wrote:
>
> On 12/17/2013 11:02 PM, Daniel Fuchs wrote:
>> On 12/17/13 6:02 PM, Peter Levart wrote:
>>> On 12/17/2013 05:26 PM, Mandy Chung wrote:
>>>> This is a good point.   The patch for JDK 8 and above uses limited
>>>> doPrivileged that only grants LoggingPermission("control") access even
>>>> though the system class has all permissions and it should be no
>>>> difference than the current implementation.   When we backport to 7u,
>>>> it would be an issue.
>>>
>>> I think it's an issue for limited doPrivileged use too (JDK 8).
>>> LoggingPermission("control") is a permission used to protect the whole
>>> logging system. So what was not possible with 'sealed' field would be
>>> possible with doPermission's temporary elevation of privilege even if it
>>> is just for LoggingPermission("control").
>>
>> Hi Peter,
>>
>> I don't think that is an issue - because if Handler is subclassed
>> and the custom subclass attempts to call something requiring
>> LoggingPermission("control") then the custom subclass will
>> *still need* to  have the "control" permission  - otherwise a
>> security exception will be  thrown. This is because the
>> custom subclass's protection domain will be in the code
>> path:
>>
>> - callers call new XxxxHandler
>>   - XxxxHandler call Handler's super constructor
>>      - super constructor calls doPrivileged
>>        - super constructor calls e.g. this.setLevel()
>>          which is redefined as XxxxHandler.setLevel
>>          - setLevel calls something else requiring "control"
>>
>> At this point you have XxxxHandler.class in the code path so
>> the security manager will check that XxxxHandler.class has the
>> "control" permission.
>
> Ah, I see. Thanks for the explanation. I haven't thought of different
> protection domains. I have studied the AccessControler javadocs and I
> think I understand the behavior now. So there's no problem even if
> unlimited doPrivileged is used, or am I missing something?
>
> But then we have another problem with doPrivileged approach, since it is
> even more restrictive than 'sealed' field approach. Currently the
> Handler's subclass that overrides a setter and calls super, works:
>
>
> @Override
> public void setOutputStream(OutputStream out) {
>      super.setOutputStream(out);
> }
>
>
> With doPrivileged wrapping setOutputStream in the superclass
> constructor, it will throw SecurityException if the subclass protection
> domain does not have the "control" permission...
>
> Can this break custom handlers in some environment?

Good question. It may yes - but on the other hand, a subclass of
handler that override such methods and call the super.method would
*need* to have the control permission too - wouldn't it?

Otherwise calling these methods when sealed is back to true would
always fail...

best regards,

-- daniel

>
>
> Regards, Peter
>
>>
>>>> We shall look into this and it's an existing issue to the current
>>>> implementation.
>>>
>>> I don't think so. The 'sealed' field only pertains to permission checks
>>> inside the Handler instance - not globally. I think we must fix the bug
>>> by keeping 'sealed' field. One variant is by having final 'sealed'
>>> field(s) in each Handler's subclass (as proposed initialy), the other
>>> would be to replace the primitive boolean sealed field in Handler with:
>>>
>>> final AtomicBoolean sealed = new AtomicBoolean(true);
>>
>> I don't think that would solve anything - it wouldn't prevent
>> the race condition - would it?
>>
>>>
>>> Which one do you prefer?
>>
>> I think we should fix the potential race condition using
>> limited doPrivileged in 9. IMHO that does exactly what we
>> want.
>>
>> best regards,
>>
>> -- daniel
>>>
>>> Regards, Peter
>>>
>>>>
>>>> thanks
>>>> Mandy
>>>>
>>>> On 12/17/2013 7:29 AM, Jason Mehrens wrote:
>>>>> Handlers can be subclassed.  Is it a security concern when
>>>>> doPrivileged is invoking non-final public/protected methods?  For
>>>>> example,
>>>>>
>>>>> @Override
>>>>> public void setOutputStream(OutputStream out) {
>>>>>      LogManager.getLogManager().reset();
>>>>> }
>>>>>
>>>>> @Override
>>>>> public void setLevel(Level l) {
>>>>>         LogManager.getLogManager().reset();
>>>>> }
>>>>>
>>>>> Jason
>>>>>
>>>>> > Date: Mon, 16 Dec 2013 13:14:03 -0800
>>>>> > From: mandy.chung at oracle.com
>>>>> > To: peter.levart at gmail.com
>>>>> > Subject: Re: Theoretical data race on
>>>>> java.util.logging.Handler.sealed
>>>>> > CC: core-libs-dev at openjdk.java.net
>>>>> >
>>>>> > 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 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?
>>>>> >
>>>>> > 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
>>>>> > >>
>>>>> > >
>>>>> >
>>>>
>>>
>>
>