From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers) Date: Tue, 25 Feb 2014 23:01:34 +0000 (UTC) Subject: [lttng-dev] [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient In-Reply-To: <1393363925-4773-1-git-send-email-jeremie.galarneau@efficios.com> References: <204474029.28925.1393099864388.JavaMail.zimbra@efficios.com> <1393363925-4773-1-git-send-email-jeremie.galarneau@efficios.com> Message-ID: <1234284751.30477.1393369294186.JavaMail.zimbra@efficios.com> merged, thanks! Mathieu ----- Original Message ----- > From: "J?r?mie Galarneau" > To: lttng-dev at lists.lttng.org > Sent: Tuesday, February 25, 2014 4:32:05 PM > Subject: [lttng-dev] [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient > > 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 > --- > .../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 enabledEventList = new ArrayList(); > + private Set enabledEventSet = > + Collections.synchronizedSet(new HashSet()); > /* > * 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 tmpList = new ArrayList(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 modifiedEvents = new HashSet(); > + Iterator 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 > > > _______________________________________________ > lttng-dev mailing list > lttng-dev at lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com