[Bug 55827] New: Iteration over a view on a synchronized collection without obtaining a lock on the collection

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=55827

            Bug ID: 55827
           Summary: Iteration over a view on a synchronized collection
                    without obtaining a lock on the collection
           Product: JMeter
           Version: 2.10
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Main
          Assignee: issues@jmeter.apache.org
          Reporter: bob@contemplateltd.com

We ran our static analysis tool ThreadSafe [1] on version 2.10 of JMeter, which
appeared to uncover a couple of concurrency issues. One of the most interesting
was the possibility of an unsynchronised iteration over a synchronised
collection in the class 'AbstractTestElement'.

The relevant line is 499:
   Iterator<Map.Entry<String, JMeterProperty>>  iter =
propMap.entrySet().iterator();

where the propMap field always contains a synchronised collection created at
line 57 in the same file.

The JDK documentation for Collections.synchronizedMap states that:

>  It is imperative that the user manually synchronize on the
> returned map when iterating over any of its collection views:
>
>  Map m = Collections.synchronizedMap(new HashMap());
>      ...
>  Set s = m.keySet();  // Needn't be in synchronized block
>      ...
>  synchronized (m) {  // Synchronizing on m, not s!
>      Iterator i = s.iterator(); // Must be in synchronized block
>      while (i.hasNext())
>          foo(i.next());
>  }
>
> Failure to follow this advice may result in non-deterministic behavior. 

It appears that the code on line 499 does not correctly synchronize on propMap,
leading to the possibility of the non-deterministic behaviour the JDK
documentation warns about.

We're not sure that this can actually result in a user-visible bug, but we
thought you'd like to know.

We are also planning to use this finding as an example of Android-related
concurrency mistakes in an article about ThreadSafe. Obviously, if you, as the
developers of JMeter, have any objections to our using this as an example, then
we won't.

[1] ThreadSafe is a static analysis tool for Java concurrency, developed by
Contemplate Ltd.:
     http://www.contemplateltd.com/

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 55827] Iteration over a view on a synchronized collection without obtaining a lock on the collection

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=55827

--- Comment #1 from Bob Atkey <bo...@contemplateltd.com> ---
I wrote:

> We are also planning to use this finding as an example of Android-related 
> concurrency mistakes in an article about ThreadSafe.

Obviously, "Android-related" was a mistake here. I meant to write
"Java-related". Sorry about that.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 55827] Iteration over a view on a synchronized collection without obtaining a lock on the collection

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=55827

Philippe Mouawad <p....@ubik-ingenierie.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |NEW

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 55827] Iteration over a view on a synchronized collection without obtaining a lock on the collection

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=55827

--- Comment #3 from Bob Atkey <bo...@contemplateltd.com> ---
> Thanks for report, I was wondering why your tool does not also report line 456:
> return new PropertyIteratorImpl(propMap.values());

Actually, our tool doesn't report the instance on line 456 because it doesn't
look into the implementation of PropertyIteratorImpl while analysing the
AbstractTestElement class. Therefore, it doesn't realise that everything that
is passed in will have .iterator() called on it. This is certainly something
that we will look at adding to ThreadSafe in the future. Thanks!

> Did you report all findings of your tool or only some examples ?

I only reported two examples of findings that I thought looked interesting. If
I get more time soon I will report the rest of the interesting looking reports
that ThreadSafe gives.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 55827] Iteration over a view on a synchronized collection without obtaining a lock on the collection

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=55827

Philippe Mouawad <p....@ubik-ingenierie.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO
                 CC|                            |p.mouawad@ubik-ingenierie.c
                   |                            |om

--- Comment #2 from Philippe Mouawad <p....@ubik-ingenierie.com> ---
Thanks for report, I was wondering why your tool does not also report line 456:
return new PropertyIteratorImpl(propMap.values());


Did you report all findings of your tool or only some examples ?

-- 
You are receiving this mail because:
You are the assignee for the bug.