activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiri Danek <jda...@redhat.com>
Subject Re: [DISCUSS] Coverity Scan for Artemis
Date Sat, 25 Feb 2017 08:11:06 GMT
Hello, let me sent one more e-mail in this thread ;P Trying to evaluate
what Coverity found, I opened some Jiras where I understood the problem and
it seemed real, and marked the Coverity find as ignored if .

The Jiras are

    ARTEMIS-996 Simplify and deduplicate lookupHome(path) in
artemis-maven-plugin
    ARTEMIS-993 ClientConsumerImpl.java contains unreachable code
    ARTEMIS-991 Null dereference after hitting Ctrl+d when prompted for
password in `artemis create`

There I one thing that I know about but not reported, about if you put
"_AMQ_NULL" as value for a float property in a message to the xml used in
`artemis data imp`, you get an exception out of the import command.

At this point, I think I am through all I can judge, but still a lot of
finds still remains to be acted upon. For example, I did not investigate
anything very hard; if it was not obvious or seemed easy to figure out, I
skipped it. I also skipped over all multithreading finds as a matter of
principle, because I don't have experience with multithreaded Java. If
anybody wants to look at it more, please go ahead.

Few random examples of the multithreading finds

86         if (tx != null) {

CID 1409940 (#1 of 1): Thread deadlock (LOCK_INVERSION)4. lock_order: Acquiring
lock TransactionImpl.timeoutLock while holding DuplicateIDCacheImpl.this
conflicts with the lock order established elsewhere. (The virtual call
resolves to
org.apache.activemq.artemis.core.transaction.impl.TransactionImpl.markAsRollbackOnly
.) [show details
<https://scan7.coverity.com/eventId=2434490-3&modelId=2434490-0&fileInstanceId=9909080&filePath=%2Fartemis-server%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Factivemq%2Fartemis%2Fcore%2Ftransaction%2Fimpl%2FTransactionImpl.java&fileStart=459&fileEnd=477>
]
187            tx.markAsRollbackOnly(new ActiveMQDuplicateIdException());

----------------------------------------------------------------------------------------------------------------------------

261      finally {

CID 1409057: Unguarded write (GUARDED_BY_VIOLATION) [select issue
<https://scan7.coverity.com/defectInstanceId=2430696&fileInstanceId=9893725&mergedDefectId=1409057>
]

5. thread2_modifies_field: Thread2 sets dispatching to a new value. Note
that this write can be reordered at runtime to occur before instructions
that do not access this field, even into (but not to the other side of)
preceding locked regions. Control is switched back to Thread1.

CID 1409135 (#1 of 1): Check of thread-shared field evades lock acquisition
(LOCK_EVASION)6. thread1_overwrites_value_in_field: Thread1 sets dispatching
to a new value. Now the two threads have an inconsistent view of dispatching
and updates to fields correlated with dispatching may be lost.

Guard the modification of dispatching and the read used to decide whether
to modify dispatching with the same set of locks.
262         dispatching = false;

----------------------------------------------------------------------------------------------------------------------------

148   public int getPercentageBlocked() {

CID 1409537 (#1 of 1): Result is not floating-point
(UNINTENDED_INTEGER_DIVISION)integer_division: Dividing integer expressions
flowControlInfo.getSendsBlocked() and flowControlInfo.getTotalSends(), and
then converting the integer quotient to type double. Any remainder, or
fractional part of the quotient, is ignored.

To compute and use a non-integer quotient, change or cast either operand to
type double. If integer division is intended, consider indicating that by
casting the result to type long .
149      double value = flowControlInfo.getSendsBlocked() / flowControlInfo.
getTotalSends();
150      return (int) value * 100;
151   }

----------------------------------------------------------------------------------------------------------------------------

 68   public void start() throws SQLException {

1. assign_to_field: The expression connect() assigns a new value to this
.connection, a field whose contents are used as a lock. The locking
behavior of this function may allow this assignment to occur multiple times.
 [show details
<https://scan7.coverity.com/eventId=2509558-0&modelId=2509558-0&fileInstanceId=10277616&filePath=%2Fmnt%2Fcov%2Factivemq-artemis%2Fartemis-jdbc-store%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Factivemq%2Fartemis%2Fjdbc%2Fstore%2Fdrivers%2FAbstractJDBCDriver.java&fileStart=102&fileEnd=131>
]
 69      connect();

2. lock_acquire: Acquiring lock AbstractJDBCDriver.connection.

CID 1409988 (#1 of 1): Bad choice of lock object (BAD_LOCK_OBJECT)3.
lock_on_assigned_field: Locking on the object referenced by field connection.
This lock acquisition may race with another thread assigning to this field.
The contents of connection may change while a thread is inside the critical
section, allowing two threads to enter the critical section simultaneously.

Instead of using connection as a lock, create a final field of type Object
which is only used as a lock.
 70      synchronized (connection) {
 71         createSchema();
 72         prepareStatements();
 73      }
 74   }

Cheers,
-- 
Jiří Daněk

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message