From mboxrd@z Thu Jan  1 00:00:00 1970
From: jeremie.galarneau@efficios.com (=?ISO-8859-1?Q?J=E9r=E9mie_Galarneau?=)
Date: Sat, 22 Feb 2014 14:15:15 -0500
Subject: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in
	LTTngTCPSessiondClient
In-Reply-To: <1530478584.28856.1393085579318.JavaMail.zimbra@efficios.com>
References: <1392913353-13369-1-git-send-email-jeremie.galarneau@efficios.com>
	<1530478584.28856.1393085579318.JavaMail.zimbra@efficios.com>
Message-ID: <CA+jJMxs549OwuD6QBvF6nHW8M0tHyZxcMb0oHJ6okCt4TdoY_Q@mail.gmail.com>

On Sat, Feb 22, 2014 at 11:12 AM, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
> ----- Original Message -----
>> From: "J?r?mie Galarneau" <jeremie.galarneau@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>());
>>       /*
>>        * 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) {
>> +                                     tmpList = new ArrayList<LTTngEvent>(enabledEventList);
>> +                             }
>
>
> Further down in this function, we find:
>
>                                         ret = enableCmd.enableLogger(handler, event, enabledLoggers);
>                                         if (ret == 1) {
>                                                 /* Enabled so remove the event from the list. */
>                                                 enabledEventList.remove(event);
>                                         }
>
> should we encompass a larger part of this function with synchronize ?

AFAIU, this operation is already synchronized by the synchronizedList class.

> What happens if event is removed from enabledEventList while we iterate on the list copy ?

I'm not sure I understand your concern.

Regards,
J?r?mie

>
> Thanks,
>
> MAthieu
>
>
>>
>>                               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);
>> +                                                     }
>>                                               }
>>                                       }
>>                                       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



-- 
J?r?mie Galarneau
EfficiOS Inc.
http://www.efficios.com