On Sat, Feb 22, 2014 at 11:12 AM, Mathieu Desnoyers
<[email protected]> wrote:
> ----- Original Message -----
>> From: "Jérémie Galarneau" <[email protected]>
>> To: [email protected]
>> 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 <[email protected]>
>> ---
>> .../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
>> [email protected]
>> 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
_______________________________________________
lttng-dev mailing list
[email protected]
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
