On 24/03/16 18:40, Mandy Chung wrote:
On Mar 24, 2016, at 7:52 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
Hi Mandy,
Here is the new webrev - I have added comments for the
string constants as well as for the permissions passed
through to the limited doPrivileged call:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.02/index.html
I hope that it makes it more clear that the
java.util.logging.* properties are only used when java.logging
is present, and the jdk.system.logger.* properties when
java.logging is not present.
It looks okay. “jdk.logger.packages” is to specify which packages to be
filtered and maybe better to rename it to “jdk.logger.filtered.packages”?
I am thinking about removing this property.
This was added before StackWalker was used. Now that we
have StackWalker, the inferCaller code will skip classes
that implement System.Logger - and this may be enough for
our purposes...
It's something I still need to ponder a bit more, so
I'd suggest handling the renaming or removing in a followup fix.
About usage of limited doPrivileged, it’d be useful to have some guideline of
using limited doPrivileged - I brought this up to Jeff when it was introduced
and will ping him again.
Like this example (in SimpleConsoleLogger.java)
476 String format = AccessController.doPrivileged(
477 new GetPropertyAction(key), null,
482 new PropertyPermission(DEFAULT_FORMAT_PROP_KEY,
"read"),
487 new PropertyPermission(JUL_FORMAT_PROP_KEY, "read"));
The doPrivileged block simply reads a system property and one single
security-sensitive operation. It’s nothing wrong using limited doPrivileged in
this case but I think it’s unnecessary. Limited doPrivileged would benefit to
limiting a block of code that is hard to tell what privileges are elevated.
This is precisely one of these cases where limited doPrivileged
should be used: otherwise you could abuse this method to read an
arbitrary property.
getSimpleFormat should probably check the given key argument is either
DEFAULT_FORMAT_PROP_KEY or JUL_FORMAT_PROP_KEY; otherwise throws IAE.
Otherwise, this method would pass if key is unexpected key if security manager
is absent but fails if security manager is present.
Oh - right - good idea. Replace the limited doPrivileged with
explicit argument checking :-) Let's do that.
Thanks for the comments!
-- daniel
Mandy
