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

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

----- Original Message -----
> From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers at efficios.com>
> Cc: "David Goulet" <dgoulet at efficios.com>, lttng-dev at lists.lttng.org
> Sent: Saturday, February 22, 2014 2:15:15 PM
> Subject: Re: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
> 
> 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.

Actually, I got that the remove operation has the synchronize
implicit. Also, it should be OK if the event is in the list,
and if it is not. Moreover, the concurrent thread only do "add",
never remove, so if it was in the list copy, it will be there for
the remove.

> 
> > What happens if event is removed from enabledEventList while we iterate on
> > the list copy ?
> 
> I'm not sure I understand your concern.

It's OK, see my reply above.

I'll do another reply to your patch for other comments.

Thanks,

Mathieu

> 
> 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
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com