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

[Peter Levart]

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?


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