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

[mathieu.desnoyers@efficios.com (Mathieu Desnoyers)]

merged, thanks!

Mathieu

----- Original Message -----
> From: "J?r?mie Galarneau" <jeremie.galarneau@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