Logger initialization regression fix and a little story on security contexts during class initialization

Logger initialization regression fix and a little story on security contexts during class initialization

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

Hi,

We had an interesting regression with the logging code in GNU Classpath.
It wasn't immediately apparent because it was only caused with certain
compilers. The class initialization order was subtly different between
byte code compiled with gcj -C and jikes. So it didn't show up with gcj
-C which I used when committing this code.

The mauve test pointed out that an unexpected SecurityManager check was
made in some cases. Those cases were when the Logger class was
initialized for the first time. This is an interesting case since
depending on how/where a class is first initialized its static
constructor will be run in different security context. So the fix is to
initialize the global Logger in a PrivilegedAction to make sure that
even if the first class initialization is done through a untrusted
context the static fields can be initialized. What makes this really
awkward is that if a class is initialized from a untrusted context for
the first time and throws a SecurityException from its static
constructor that class can then NEVER be initialized again, even if it
is loaded through a privileged context.

2006-05-11  Mark Wielaard  <mark@klomp.org>

    * java/util/logging/Logger.java (global): Initialize inside static
    PrivilegedAction.

Unfortunately this patch only solves the real world scenario. It doesn't
solve the mauve failure since the security manager used in the test
doesn't actually use the security context to see whether the calls are
made with the right privileges. It is actually hard to figure out how to
test for this situation correctly. Because depends on when exactly the
class is first initialized. So for now I just make sure that the logging
framework is completely initialized before installing the test security
manager.

2006-05-11  Mark Wielaard  <mark@klomp.org>

    * gnu/testlet/java/util/logging/Handler/TestSecurityManager.java
    (install): Initialize LogManager first.
    * gnu/testlet/java/util/logging/Logger/TestSecurityManager.java
    (install): Likewise.

What we really need is a test that is run with minimal privileges and
install a real security manager that respect the whole security context
(through AccessController) which then tries to load every class to see
whether or not they can at least be initialized properly from such a
minimal security context. The problem with that is that while setting up
such a test part of the core classes are already initialized of course.
So it is hard to get anything that really tests it all.

Both patches have been committed to classpath and mauve. The classpath
patch will also go onto the release branch.

Cheers,

Mark

[-- Attachment #2: logging-init.patch --]
[-- Type: text/x-patch, Size: 1150 bytes --]

Index: java/util/logging/Logger.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/util/logging/Logger.java,v
retrieving revision 1.13
diff -u -r1.13 Logger.java
--- java/util/logging/Logger.java	3 Apr 2006 08:59:53 -0000	1.13
+++ java/util/logging/Logger.java	11 May 2006 12:10:58 -0000
@@ -41,6 +41,8 @@
 import java.util.List;
 import java.util.MissingResourceException;
 import java.util.ResourceBundle;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 
 /**
  * A Logger is used for logging information about events. Usually, there
@@ -76,7 +78,20 @@
    * products are supposed to create and use their own Loggers, so
    * they can be controlled individually.
    */
-  public static final Logger global = getLogger("global");
+  public static final Logger global;
+
+  static
+    {
+      // Our class might be initialized from an unprivileged context
+      global = (Logger) AccessController.doPrivileged
+	(new PrivilegedAction()
+	  {
+	    public Object run()
+	    {
+	      return getLogger("global");
+	    }
+	  });
+    }
 
 
   /**

[-- Attachment #3: mauve-logging-security.patch --]
[-- Type: text/x-patch, Size: 1968 bytes --]

Index: gnu/testlet/java/util/logging/Handler/TestSecurityManager.java
===================================================================
RCS file: /cvs/mauve/mauve/gnu/testlet/java/util/logging/Handler/TestSecurityManager.java,v
retrieving revision 1.1
diff -u -r1.1 TestSecurityManager.java
--- gnu/testlet/java/util/logging/Handler/TestSecurityManager.java	10 Mar 2004 23:20:42 -0000	1.1
+++ gnu/testlet/java/util/logging/Handler/TestSecurityManager.java	11 May 2006 12:25:48 -0000
@@ -24,7 +24,7 @@
 import java.security.Permission;
 import java.security.AccessControlException;
 import java.util.logging.LoggingPermission;
-
+import java.util.logging.LogManager;
 
 /**
  * A SecurityManager that can be told whether or not to grant
@@ -57,6 +57,9 @@
 
   public void install()
   {
+    // Make sure the LogManager is fully installed first.
+    LogManager lm = LogManager.getLogManager();
+
     SecurityManager oldsm = System.getSecurityManager();
     
     if (oldsm == this)
Index: gnu/testlet/java/util/logging/Logger/TestSecurityManager.java
===================================================================
RCS file: /cvs/mauve/mauve/gnu/testlet/java/util/logging/Logger/TestSecurityManager.java,v
retrieving revision 1.2
diff -u -r1.2 TestSecurityManager.java
--- gnu/testlet/java/util/logging/Logger/TestSecurityManager.java	10 May 2004 14:50:51 -0000	1.2
+++ gnu/testlet/java/util/logging/Logger/TestSecurityManager.java	11 May 2006 12:25:48 -0000
@@ -24,7 +24,7 @@
 import java.security.Permission;
 import java.security.AccessControlException;
 import java.util.logging.LoggingPermission;
-
+import java.util.logging.LogManager;
 
 /**
  * A SecurityManager that can be told whether or not to grant
@@ -57,6 +57,9 @@
 
   public void install()
   {
+    // Make sure the LogManager is fully installed first.
+    LogManager lm = LogManager.getLogManager();
+
     SecurityManager oldsm = System.getSecurityManager();
     
     if (oldsm == this)

Re: [cp-patches] Logger initialization regression fix and a little story on security contexts during class initialization

[Archie Cobbs]

Mark Wielaard wrote:
> We had an interesting regression with the logging code in GNU Classpath.
> It wasn't immediately apparent because it was only caused with certain
> compilers. The class initialization order was subtly different between
> byte code compiled with gcj -C and jikes. So it didn't show up with gcj
> -C which I used when committing this code.

Since class initialization is strictly defined by the JVM spec,
doesn't this necessarily imply a bug in either gcj or jikes?

The only other alternative is that they both compile the same code
correctly, but do so differenly enough to change the class initialization
order, which to me seems even more surprising (but possible I guess.. ?)

-Archie

__________________________________________________________________________
Archie Cobbs      *        CTO, Awarix        *      http://www.awarix.com

Re: [cp-patches] Logger initialization regression fix and a little story on security contexts during class initialization

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

Hi Archie,

On Thu, 2006-05-11 at 08:51 -0500, Archie Cobbs wrote:
> Mark Wielaard wrote:
> > We had an interesting regression with the logging code in GNU Classpath.
> > It wasn't immediately apparent because it was only caused with certain
> > compilers. The class initialization order was subtly different between
> > byte code compiled with gcj -C and jikes. So it didn't show up with gcj
> > -C which I used when committing this code.
> 
> Since class initialization is strictly defined by the JVM spec,
> doesn't this necessarily imply a bug in either gcj or jikes?
> 
> The only other alternative is that they both compile the same code
> correctly, but do so differenly enough to change the class initialization
> order, which to me seems even more surprising (but possible I guess.. ?)

Yeah, I didn't really investigate further since the issue was real and
not just a contrived case in Mauve. We need to make sure that if a class
is initialized from a non-trusted environment the static constructor
runs correctly.

Cheers,

Mark

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]