6823527: java.util.logging.Handler has thread safety issues

Reviewed-by: dholmes, mchung

src/share/classes/java/util/logging/ConsoleHandler.java

@@ -26,9 +26,6 @@
 
 package java.util.logging;
 
-import java.io.*;
-import java.net.*;
-
 /**
  * This <tt>Handler</tt> publishes log records to <tt>System.err</tt>.
  * By default the <tt>SimpleFormatter</tt> is used to generate brief summaries.
@@ -114,6 +111,7 @@ public class ConsoleHandler extends StreamHandler {
      * @param  record  description of the log event. A null record is
      *                 silently ignored and is not published
      */
+    @Override
     public void publish(LogRecord record) {
         super.publish(record);
         flush();
@@ -124,6 +122,7 @@ public class ConsoleHandler extends StreamHandler {
      * to close the output stream.  That is, we do <b>not</b>
      * close <tt>System.err</tt>.
      */
+    @Override
     public void close() {
         flush();
     }

src/share/classes/java/util/logging/FileHandler.java

@@ -149,7 +149,7 @@ public class FileHandler extends StreamHandler {
     private FileChannel lockFileChannel;
     private File files[];
     private static final int MAX_LOCKS = 100;
-    private static java.util.HashMap<String, String> locks = new java.util.HashMap<>();
+    private static final java.util.HashMap<String, String> locks = new java.util.HashMap<>();
 
     /**
      * A metered stream is a subclass of OutputStream that
@@ -157,7 +157,7 @@ public class FileHandler extends StreamHandler {
      * (b) keeps track of how many bytes have been written
      */
     private class MeteredStream extends OutputStream {
-        OutputStream out;
+        final OutputStream out;
         int written;
 
         MeteredStream(OutputStream out, int written) {
@@ -165,25 +165,30 @@ public class FileHandler extends StreamHandler {
             this.written = written;
         }
 
+        @Override
         public void write(int b) throws IOException {
             out.write(b);
             written++;
         }
 
+        @Override
         public void write(byte buff[]) throws IOException {
             out.write(buff);
             written += buff.length;
         }
 
+        @Override
         public void write(byte buff[], int off, int len) throws IOException {
             out.write(buff,off,len);
             written += len;
         }
 
+        @Override
         public void flush() throws IOException {
             out.flush();
         }
 
+        @Override
         public void close() throws IOException {
             out.close();
         }
@@ -607,6 +612,7 @@ public class FileHandler extends StreamHandler {
      * @param  record  description of the log event. A null record is
      *                 silently ignored and is not published
      */
+    @Override
     public synchronized void publish(LogRecord record) {
         if (!isLoggable(record)) {
             return;
@@ -620,6 +626,7 @@ public class FileHandler extends StreamHandler {
             // currently being called from untrusted code.
             // So it is safe to raise privilege here.
             AccessController.doPrivileged(new PrivilegedAction<Object>() {
+                @Override
                 public Object run() {
                     rotate();
                     return null;
@@ -634,6 +641,7 @@ public class FileHandler extends StreamHandler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      */
+    @Override
     public synchronized void close() throws SecurityException {
         super.close();
         // Unlock any lock file.
@@ -656,6 +664,7 @@ public class FileHandler extends StreamHandler {
 
     private static class InitializationErrorManager extends ErrorManager {
         Exception lastException;
+        @Override
         public void error(String msg, Exception ex, int code) {
             lastException = ex;
         }

src/share/classes/java/util/logging/Handler.java

@@ -47,12 +47,20 @@ import java.io.UnsupportedEncodingException;
 
 public abstract class Handler {
     private static final int offValue = Level.OFF.intValue();
-    private LogManager manager = LogManager.getLogManager();
-    private Filter filter;
-    private Formatter formatter;
-    private Level logLevel = Level.ALL;
-    private ErrorManager errorManager = new ErrorManager();
-    private String encoding;
+    private final LogManager manager = LogManager.getLogManager();
+
+    // We're using volatile here to avoid synchronizing getters, which
+    // would prevent other threads from calling isLoggable()
+    // while publish() is executing.
+    // On the other hand, setters will be synchronized to exclude concurrent
+    // execution with more complex methods, such as StreamHandler.publish().
+    // We wouldn't want 'level' to be changed by another thread in the middle
+    // of the execution of a 'publish' call.
+    private volatile Filter filter;
+    private volatile Formatter formatter;
+    private volatile Level logLevel = Level.ALL;
+    private volatile ErrorManager errorManager = new ErrorManager();
+    private volatile String encoding;
 
     // Package private support for security checking.  When sealed
     // is true, we access check updates to the class.
@@ -110,7 +118,7 @@ public abstract class Handler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      */
-    public void setFormatter(Formatter newFormatter) throws SecurityException {
+    public synchronized void setFormatter(Formatter newFormatter) throws SecurityException {
         checkPermission();
         // Check for a null pointer:
         newFormatter.getClass();
@@ -138,7 +146,7 @@ public abstract class Handler {
      * @exception  UnsupportedEncodingException if the named encoding is
      *          not supported.
      */
-    public void setEncoding(String encoding)
+    public synchronized void setEncoding(String encoding)
                         throws SecurityException, java.io.UnsupportedEncodingException {
         checkPermission();
         if (encoding != null) {
@@ -174,7 +182,7 @@ public abstract class Handler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      */
-    public void setFilter(Filter newFilter) throws SecurityException {
+    public synchronized void setFilter(Filter newFilter) throws SecurityException {
         checkPermission();
         filter = newFilter;
     }
@@ -198,7 +206,7 @@ public abstract class Handler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      */
-    public void setErrorManager(ErrorManager em) {
+    public synchronized void setErrorManager(ErrorManager em) {
         checkPermission();
         if (em == null) {
            throw new NullPointerException();
@@ -264,7 +272,7 @@ public abstract class Handler {
      * than this level will be discarded.
      * @return  the level of messages being logged.
      */
-    public synchronized Level getLevel() {
+    public Level getLevel() {
         return logLevel;
     }
 
@@ -282,11 +290,11 @@ public abstract class Handler {
      *
      */
     public boolean isLoggable(LogRecord record) {
-        int levelValue = getLevel().intValue();
+        final int levelValue = getLevel().intValue();
         if (record.getLevel().intValue() < levelValue || levelValue == offValue) {
             return false;
         }
-        Filter filter = getFilter();
+        final Filter filter = getFilter();
         if (filter == null) {
             return true;
         }

src/share/classes/java/util/logging/MemoryHandler.java

@@ -88,7 +88,7 @@ package java.util.logging;
 
 public class MemoryHandler extends Handler {
     private final static int DEFAULT_SIZE = 1000;
-    private Level pushLevel;
+    private volatile Level pushLevel;
     private int size;
     private Handler target;
     private LogRecord buffer[];
@@ -188,6 +188,7 @@ public class MemoryHandler extends Handler {
      * @param  record  description of the log event. A null record is
      *                 silently ignored and is not published
      */
+    @Override
     public synchronized void publish(LogRecord record) {
         if (!isLoggable(record)) {
             return;
@@ -227,6 +228,7 @@ public class MemoryHandler extends Handler {
      * Note that the current contents of the <tt>MemoryHandler</tt>
      * buffer are <b>not</b> written out.  That requires a "push".
      */
+    @Override
     public void flush() {
         target.flush();
     }
@@ -238,6 +240,7 @@ public class MemoryHandler extends Handler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      */
+    @Override
     public void close() throws SecurityException {
         target.close();
         setLevel(Level.OFF);
@@ -252,11 +255,10 @@ public class MemoryHandler extends Handler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      */
-    public void setPushLevel(Level newLevel) throws SecurityException {
+    public synchronized void setPushLevel(Level newLevel) throws SecurityException {
         if (newLevel == null) {
             throw new NullPointerException();
         }
-        LogManager manager = LogManager.getLogManager();
         checkPermission();
         pushLevel = newLevel;
     }
@@ -266,7 +268,7 @@ public class MemoryHandler extends Handler {
      *
      * @return the value of the <tt>pushLevel</tt>
      */
-    public synchronized Level getPushLevel() {
+    public Level getPushLevel() {
         return pushLevel;
     }
 
@@ -283,6 +285,7 @@ public class MemoryHandler extends Handler {
      * @return true if the <tt>LogRecord</tt> would be logged.
      *
      */
+    @Override
     public boolean isLoggable(LogRecord record) {
         return super.isLoggable(record);
     }

src/share/classes/java/util/logging/SocketHandler.java

@@ -82,7 +82,6 @@ public class SocketHandler extends StreamHandler {
     private Socket sock;
     private String host;
     private int port;
-    private String portProperty;
 
     // Private method to configure a SocketHandler from LogManager
     // properties and/or default values as specified in the class
@@ -177,6 +176,7 @@ public class SocketHandler extends StreamHandler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      */
+    @Override
     public synchronized void close() throws SecurityException {
         super.close();
         if (sock != null) {
@@ -195,6 +195,7 @@ public class SocketHandler extends StreamHandler {
      * @param  record  description of the log event. A null record is
      *                 silently ignored and is not published
      */
+    @Override
     public synchronized void publish(LogRecord record) {
         if (!isLoggable(record)) {
             return;

src/share/classes/java/util/logging/StreamHandler.java

@@ -73,10 +73,9 @@ import java.io.*;
  */
 
 public class StreamHandler extends Handler {
-    private LogManager manager = LogManager.getLogManager();
     private OutputStream output;
     private boolean doneHeader;
-    private Writer writer;
+    private volatile Writer writer;
 
     // Private method to configure a StreamHandler from LogManager
     // properties and/or default values as specified in the class
@@ -169,7 +168,8 @@ public class StreamHandler extends Handler {
      * @exception  UnsupportedEncodingException if the named encoding is
      *          not supported.
      */
-    public void setEncoding(String encoding)
+    @Override
+    public synchronized void setEncoding(String encoding)
                         throws SecurityException, java.io.UnsupportedEncodingException {
         super.setEncoding(encoding);
         if (output == null) {
@@ -201,6 +201,7 @@ public class StreamHandler extends Handler {
      * @param  record  description of the log event. A null record is
      *                 silently ignored and is not published
      */
+    @Override
     public synchronized void publish(LogRecord record) {
         if (!isLoggable(record)) {
             return;
@@ -240,6 +241,7 @@ public class StreamHandler extends Handler {
      * @return true if the <tt>LogRecord</tt> would be logged.
      *
      */
+    @Override
     public boolean isLoggable(LogRecord record) {
         if (writer == null || record == null) {
             return false;
@@ -250,6 +252,7 @@ public class StreamHandler extends Handler {
     /**
      * Flush any buffered messages.
      */
+    @Override
     public synchronized void flush() {
         if (writer != null) {
             try {
@@ -294,6 +297,7 @@ public class StreamHandler extends Handler {
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have LoggingPermission("control").
      */
+    @Override
     public synchronized void close() throws SecurityException {
         flushAndClose();
     }