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
