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

[Jason Mehrens]

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