[jira] [Updated] (ARTEMIS-1152) Investigate suspected Double-Checked Locking

["Jiri Danek (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/ARTEMIS-1152?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Jiri Danek updated ARTEMIS-1152:
--------------------------------
    Attachment: ManagementServiceImpl.java.png
                ActiveMQThreadPoolExecutor.java.png

> Investigate suspected Double-Checked Locking
> --------------------------------------------
>
>                 Key: ARTEMIS-1152
>                 URL: https://issues.apache.org/jira/browse/ARTEMIS-1152
>             Project: ActiveMQ Artemis
>          Issue Type: Wish
>          Components: Broker
>    Affects Versions: 2.next
>            Reporter: Jiri Danek
>            Priority: Minor
>         Attachments: ActiveMQThreadPoolExecutor.java.png, ManagementServiceImpl.java.png
>
>
> Coverity found instance of Double-Checked Locking. According to the resources at the end, the variable has to either be declared volatile, or double-checked locking should not be used, or the variable has to be atomic (int, float, ...). Otherwise, in a general case this may lead to threads accessing partially initialized objects.
> There is 9 Coverity finds in the category "Data race undermines locking", the following one looks to me as clear examples of the Double-Checked Locking pattern
> h4. ActiveMQJMSContext.java
> {noformat}
> 130   private void checkSession() {
>    	1. thread1_checks_field: Thread1 uses the value read from field session in the condition session == null. It sees that the condition is true.
>    	
> CID 1409080 (#2-1 of 2): Check of thread-shared field evades lock acquisition (LOCK_EVASION)
> 5. thread2_checks_field_early: Thread2 checks session, reading it after Thread1 assigns to session but before some of the correlated field assignments can occur. It sees the condition session == null as being false. It continues on before the critical section has completed, and can read data changed by that critical section while it is in an inconsistent state.
>    	Remove this outer, unlocked check of session.
> 131      if (session == null) {
>    	2. thread1_acquires_lock: Thread1 acquires lock ActiveMQJMSContext.this.
> 132         synchronized (this) {
> 133            if (closed)
> 134               throw new IllegalStateRuntimeException("Context is closed");
>    	3. thread1_double_checks_field: Thread1 double checks the field session in the condition session == null.
> 135            if (session == null) {
> 136               try {
> 137                  if (xa) {
> 138                     session = ((XAConnection) connection).createXASession();
> 139                  } else {
>    	4. thread1_modifies_field: Thread1 modifies the field session. This modification can be re-ordered with other correlated field assignments within this critical section at runtime. Thus, checking the value of session is not an adequate test that the critical section has completed unless the guarding lock is held while checking. If session is assigned a newly constructed value, note that the JVM is allowed to reorder the assignment of the new reference to session before any field assignments that may occur in the constructor. Control is switched to Thread2.
> 140                     session = connection.createSession(sessionMode);
> 141                  }
> 142               } catch (JMSException e) {
> 143                  throw JmsExceptionUtils.convertToRuntimeException(e);
> 144               }
> 145            }
> 146         }
> 147      }
> 148   }
> {noformat}
> In addition to that, the demo version of HP Fortify tool reports the following two additional instances (as well as the previous one already reported by Coverity).
> h4. ActiveMQThreadPoolExecutor.java
> !ActiveMQThreadPoolExecutor.java.png!
> h4. ManagementServiceImpl.java
> !ManagementServiceImpl.java.png!
> I came to believe that this warning from the tool is real, at least the first instance, and that it should be investigated more closely by somebody more experienced.
> #. http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
> #. https://www.securecoding.cert.org/confluence/display/java/LCK10-J.+Use+a+correct+form+of+the+double-checked+locking+idiom



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)