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

[Mathieu Desnoyers]

Other comments,

----- Original Message -----
> From: "Jérémie Galarneau" <jeremie.galarneau at efficios.com>
> To: lttng-dev at lists.lttng.org
> Sent: Thursday, February 20, 2014 11:22:33 AM
> Subject: [lttng-dev] [PATCH lttng-ust] 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
> synchronizedList which implements the same interface.
> 
> Signed-off-by: Jérémie Galarneau <jeremie.galarneau at efficios.com>
> ---
>  .../org/lttng/ust/jul/LTTngTCPSessiondClient.java         | 15
>  +++++++++++----
>  1 file changed, 11 insertions(+), 4 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..0d9a485 100644
> --- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> +++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> @@ -34,6 +34,7 @@ import java.util.List;
>  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 +58,8 @@ public class LTTngTCPSessiondClient {
>  	private Semaphore registerSem;
>  
>  	private Timer eventTimer;
> -	private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
> +	private List<LTTngEvent> enabledEventList = Collections.synchronizedList(
> +		new ArrayList<LTTngEvent>());

I'd prefer:

  private List<LTTngEvent> enabledEventList =
         Collections.synchronizedList(new ArrayList<LTTngEvent>());

unless you tell me that your coding style is more "java-like".

>  	/*
>  	 * Map of Logger objects that have been enabled. They are indexed by name.
>  	 */
> @@ -86,7 +88,10 @@ public class LTTngTCPSessiondClient {
>  				 * 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);
> +				List<LTTngEvent> tmpList;
> +				synchronized(enabledEventList) {

inconsistent style, missing space after "synchronized".

> +					tmpList = new ArrayList<LTTngEvent>(enabledEventList);
> +				}
>  
>  				LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
>  					LTTngSessiondCmd2_4.sessiond_enable_handler();
> @@ -277,8 +282,10 @@ public class LTTngTCPSessiondClient {
>  						 * Add the event to the list so it can be enabled if
>  						 * the logger appears at some point in time.
>  						 */
> -						if (enabledEventList.contains(event) == false) {
> -							enabledEventList.add(event);
> +						synchronized (enabledEventList) {
> +							if (enabledEventList.contains(event) == false) {
> +								enabledEventList.add(event);
> +							}

Hrm, so what we want here is probably more a set than a list ? We want a data structure
that does not have duplicates. If we can confirm that the order of the elements does not
matter, a hash map might be a much more suited data structure than a list. We could
then remove the explicit synchronize around "add".

Thanks,

Mathieu

>  						}
>  					}
>  					data = enableCmd.getBytes();
> --
> 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