[lttng-dev] [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient
Mathieu Desnoyers
mathieu.desnoyers at efficios.com
Tue Feb 25 18:01:34 EST 2014
merged, thanks!
Mathieu
----- Original Message -----
> From: "Jérémie Galarneau" <jeremie.galarneau at efficios.com>
> 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 <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
>
>
> _______________________________________________
> 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
More information about the lttng-dev
mailing list