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

[Peter Levart]

On 12/17/2013 04:29 PM, 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

Good catch! The 'sealed' field did not allow that, since there was no 
elevation of privilege. So doPrivileged is out of question, right?

Peter

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