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

Tue Dec 17 22:02:13 UTC 2013

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.

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