Jiri Danek created ARTEMIS-1152:
-----------------------------------
Summary: 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
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)
