[lttng-dev] [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient

[Jérémie Galarneau]

enabledEventList is shared between the LTTngThread and eventTimer
threads but is not synchronized.

This patch changes enabledEventList's type from an ArrayList to a
synchronized HashSet.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau at efficios.com>
---
 .../org/lttng/ust/jul/LTTngTCPSessiondClient.java  | 129 ++++++++++++---------
 1 file changed, 74 insertions(+), 55 deletions(-)

diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
index 25d1cb2..35f768f 100644
--- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
+++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
@@ -30,10 +30,14 @@ import java.net.*;
 import java.lang.management.ManagementFactory;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
 import java.util.Timer;
 import java.util.TimerTask;
 import java.util.logging.Logger;
+import java.util.Collections;
 
 class USTRegisterMsg {
 	public static int pid;
@@ -57,7 +61,8 @@ public class LTTngTCPSessiondClient {
 	private Semaphore registerSem;
 
 	private Timer eventTimer;
-	private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
+	private Set<LTTngEvent> enabledEventSet =
+		Collections.synchronizedSet(new HashSet<LTTngEvent>());
 	/*
 	 * Map of Logger objects that have been enabled. They are indexed by name.
 	 */
@@ -82,65 +87,81 @@ public class LTTngTCPSessiondClient {
 		this.eventTimer.scheduleAtFixedRate(new TimerTask() {
 			@Override
 			public void run() {
-				/*
-				 * We have to make a copy here since it is possible that the
-				 * enabled event list is changed during an iteration on it.
-				 */
-				List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
-
-				LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
-					LTTngSessiondCmd2_4.sessiond_enable_handler();
-				for (LTTngEvent event: tmpList) {
-					int ret;
-					Logger logger;
-
+				synchronized (enabledEventSet) {
+					LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
+						LTTngSessiondCmd2_4.sessiond_enable_handler();
 					/*
-					 * Check if this Logger name has been enabled already. Note
-					 * that in the case of "*", it's never added in that hash
-					 * table thus the enable command does a lookup for each
-					 * logger name in that hash table for the * case in order
-					 * to make sure we don't enable twice the same logger
-					 * because JUL apparently accepts that the *same*
-					 * LogHandler can be added twice on a Logger object...
-					 * don't ask...
+					 * Modifying events in a Set will raise a
+					 * ConcurrentModificationException. Thus, we remove an event
+					 * and add its modified version to modifiedEvents when a
+					 * modification is necessary.
 					 */
-					logger = enabledLoggers.get(event.name);
-					if (logger != null) {
-						continue;
-					}
+					Set<LTTngEvent> modifiedEvents = new HashSet<LTTngEvent>();
+					Iterator<LTTngEvent> it = enabledEventSet.iterator();
 
-					/*
-					 * Set to one means that the enable all event has been seen
-					 * thus event from that point on must use loglevel for all
-					 * events. Else the object has its own loglevel.
-					 */
-					if (handler.logLevelUseAll == 1) {
-						event.logLevel.level = handler.logLevelAll;
-						event.logLevel.type = handler.logLevelTypeAll;
-					}
+					while (it.hasNext()) {
+						int ret;
+						Logger logger;
+						LTTngEvent event = it.next();
 
-					/*
-					 * The all event is a special case since we have to iterate
-					 * over every Logger to see which one was not enabled.
-					 */
-					if (event.name.equals("*")) {
-						enableCmd.name = event.name;
-						enableCmd.lttngLogLevel = event.logLevel.level;
-						enableCmd.lttngLogLevelType = event.logLevel.type;
 						/*
-						 * The return value is irrelevant since the * event is
-						 * always kept in the list.
+						 * Check if this Logger name has been enabled already. Note
+						 * that in the case of "*", it's never added in that hash
+						 * table thus the enable command does a lookup for each
+						 * logger name in that hash table for the * case in order
+						 * to make sure we don't enable twice the same logger
+						 * because JUL apparently accepts that the *same*
+						 * LogHandler can be added twice on a Logger object...
+						 * don't ask...
 						 */
-						enableCmd.execute(handler, enabledLoggers);
-						continue;
-					}
+						logger = enabledLoggers.get(event.name);
+						if (logger != null) {
+							continue;
+						}
 
-					ret = enableCmd.enableLogger(handler, event, enabledLoggers);
-					if (ret == 1) {
-						/* Enabled so remove the event from the list. */
-						enabledEventList.remove(event);
+						/*
+						 * Set to one means that the enable all event has been seen
+						 * thus event from that point on must use loglevel for all
+						 * events. Else the object has its own loglevel.
+						 */
+						if (handler.logLevelUseAll == 1) {
+							it.remove();
+							event.logLevel.level = handler.logLevelAll;
+							event.logLevel.type = handler.logLevelTypeAll;
+							modifiedEvents.add(event);
+						}
+
+						/*
+						 * The all event is a special case since we have to iterate
+						 * over every Logger to see which one was not enabled.
+						 */
+						if (event.name.equals("*")) {
+							enableCmd.name = event.name;
+							enableCmd.lttngLogLevel = event.logLevel.level;
+							enableCmd.lttngLogLevelType = event.logLevel.type;
+							/*
+							 * The return value is irrelevant since the * event is
+							 * always kept in the set.
+							 */
+							enableCmd.execute(handler, enabledLoggers);
+							continue;
+						}
+
+						ret = enableCmd.enableLogger(handler, event, enabledLoggers);
+						if (ret == 1) {
+							/* Enabled so remove the event from the set. */
+							if (!modifiedEvents.remove(event)) {
+								/*
+								 * event can only be present in one of
+								 * the sets.
+								 */
+								it.remove();
+							}
+						}
 					}
+					enabledEventSet.addAll(modifiedEvents);
 				}
+
 			}
 		}, this.timerDelay, this.timerDelay);
 
@@ -274,12 +295,10 @@ public class LTTngTCPSessiondClient {
 					event = enableCmd.execute(this.handler, this.enabledLoggers);
 					if (event != null) {
 						/*
-						 * Add the event to the list so it can be enabled if
+						 * Add the event to the set so it can be enabled if
 						 * the logger appears at some point in time.
 						 */
-						if (enabledEventList.contains(event) == false) {
-							enabledEventList.add(event);
-						}
+						enabledEventSet.add(event);
 					}
 					data = enableCmd.getBytes();
 					break;
-- 
1.9.0