6823527: java.util.logging.Handler has thread safety issues
Reviewed-by: dholmes, mchung
--- a/jdk/src/share/classes/java/util/logging/ConsoleHandler.java Wed Sep 04 11:40:23 2013 +0100
+++ b/jdk/src/share/classes/java/util/logging/ConsoleHandler.java Wed Sep 04 15:32:59 2013 +0200
@@ -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 @@
* @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 @@
* to close the output stream. That is, we do <b>not</b>
* close <tt>System.err</tt>.
*/
+ @Override
public void close() {
flush();
}
--- a/jdk/src/share/classes/java/util/logging/FileHandler.java Wed Sep 04 11:40:23 2013 +0100
+++ b/jdk/src/share/classes/java/util/logging/FileHandler.java Wed Sep 04 15:32:59 2013 +0200
@@ -149,7 +149,7 @@
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 @@
* (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 @@
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 @@
* @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 @@
// 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 @@
* @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 @@
private static class InitializationErrorManager extends ErrorManager {
Exception lastException;
+ @Override
public void error(String msg, Exception ex, int code) {
lastException = ex;
}
--- a/jdk/src/share/classes/java/util/logging/Handler.java Wed Sep 04 11:40:23 2013 +0100
+++ b/jdk/src/share/classes/java/util/logging/Handler.java Wed Sep 04 15:32:59 2013 +0200
@@ -47,12 +47,20 @@
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 @@
* @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 @@
* @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 @@
* @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 @@
* @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 @@
* 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 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;
}
--- a/jdk/src/share/classes/java/util/logging/MemoryHandler.java Wed Sep 04 11:40:23 2013 +0100
+++ b/jdk/src/share/classes/java/util/logging/MemoryHandler.java Wed Sep 04 15:32:59 2013 +0200
@@ -88,7 +88,7 @@
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 @@
* @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 @@
* 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 @@
* @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 @@
* @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 @@
*
* @return the value of the <tt>pushLevel</tt>
*/
- public synchronized Level getPushLevel() {
+ public Level getPushLevel() {
return pushLevel;
}
@@ -283,6 +285,7 @@
* @return true if the <tt>LogRecord</tt> would be logged.
*
*/
+ @Override
public boolean isLoggable(LogRecord record) {
return super.isLoggable(record);
}
--- a/jdk/src/share/classes/java/util/logging/SocketHandler.java Wed Sep 04 11:40:23 2013 +0100
+++ b/jdk/src/share/classes/java/util/logging/SocketHandler.java Wed Sep 04 15:32:59 2013 +0200
@@ -82,7 +82,6 @@
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 @@
* @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 @@
* @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;
--- a/jdk/src/share/classes/java/util/logging/StreamHandler.java Wed Sep 04 11:40:23 2013 +0100
+++ b/jdk/src/share/classes/java/util/logging/StreamHandler.java Wed Sep 04 15:32:59 2013 +0200
@@ -73,10 +73,9 @@
*/
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 @@
* @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 @@
* @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 @@
* @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 @@
/**
* Flush any buffered messages.
*/
+ @Override
public synchronized void flush() {
if (writer != null) {
try {
@@ -294,6 +297,7 @@
* @exception SecurityException if a security manager exists and if
* the caller does not have LoggingPermission("control").
*/
+ @Override
public synchronized void close() throws SecurityException {
flushAndClose();
}