Hi Peter,
I had a look at your later webrev 03 and it looks like a good
solution to fix the issue. I am glad to see the sealed
variable removed.
About using lambda I don't know whether we have guidelines
for that - in this case it certainly makes the code more
concise. It is good to remember that it might mean more
work in backporting fixes though - for those fixes that will
need to be backported.
It will be good to hear about Mandy's opinion...
best regards,
-- daniel
On 12/8/13 8:19 PM, Peter Levart wrote:
On 12/04/2013 12:27 AM, Mandy Chung wrote:
On 12/3/2013 1:44 AM, Peter Levart wrote:
On 12/03/2013 09:51 AM, Peter Levart wrote:
Hi,
While browsing the code of java.util.logging.Handler, I noticed a
theoretical possibility that a security check in a
j.u.l.StreamHandler be circumvented using a data race.
There is a plain boolean instance field 'sealed' in j.u.l.Handler
that is pre-initialized to 'true' in field initializer.
StreamHandler sublcass' constructors overwrite this value with
'false' at the beginning, then issue some operations which
circumvent security checks, and finally they reset the 'sealed'
value back to 'true' at the end.
If a reference to an instance of StreamHandler or subclass is passed
to some thread without synchronization via data-race, this thread
can see 'true' or 'false' as the possible values of 'sealed'
variable, thus it is possible to circumvent security checks.
One possibility to fix this is moving the field to StreamHandler and
making it final:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.01/
Just making the field volatile might not work. There is an ongoing
debate on concurrency-interest which suggests that volatile fields
are not exceptional in constructors like final fields are...
Regards, Peter
The proposed patch is not complete. There are several subclasses of
StreamHandler in the java.util.logging package that also need a way
to bypass security checks for some operations in their constructors.
So here's the updated webrev which updates them with the same code as
StreamHandler. This means that there are two copies of 'sealed' flag
in object of type ConsoleHandler, for example, but only the one
declared in ConsoleHandler is relevant for governing access checks:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.02/
Before filing the bug, I'm asking the list whether this can be
considered a bug...
This does look a possible data race that might return a partially
constructed object with sealed = false. I am not sure how likely we
will run into this race though.
W.r.t. the patch, it might be better to get rid of the sealed field
and wrap the calls with doPrivileged with limited privilege (just
LoggingPermission("control"))
Mandy
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/
Regards, Peter
