Field eventsToDeliver is a LinkedList, i.e., not thread-safe. Accesses to field eventsToDeliver are protected by synchronization on itself, but not in 1 location.

[adriannistor]

I created a Pull Request for this report at:

#1971

Description

Field eventsToDeliver is a LinkedList (line 128), i.e., not thread-safe.

Accesses to eventsToDeliver are protected by synchronization on itself (synchronized (eventsToDeliver)), e.g., at lines: 373-376, 424-426, 493-494, 493-498, and 493-506.

Note that line 506 (a remove()) and line 374 (decoder calls handleMessage() as per decoder's definition at line 110, and handleMessage() calls add() at line 271) modify the eventsToDeliver (and are protected by synchronization at line 493 and line 373 respectively).

However, line 397 (an add(), i.e., modifies eventsToDeliver) is not protected.

I see line 397 adds a ON_COMPLETE_EVENT, which, when processed in the eventsToDeliver, will signal termination (in isCompletedOrDeliverEvent() at line 494).

However, I think the add() at line 397 can occur in parallel with the remove() at line 506. This is because the remove() at line 506 occurs for other messages in the eventsToDeliver that were already enqueued before the ON_COMPLETE_EVENT.

I.e., when ON_COMPLETE_EVENT is being added by line 397 other messages may still be getting processed from the eventsToDeliver queue.

Note how isCompletedOrDeliverEvent() is getting called (by itself, recursively) at lines 505-509 until eventsToDeliver becomes empty (line 498) or ON_COMPLETE_EVENT is encountered (line 494).

Note also how the recursive calls at lines 505-509 are asynchronous (which is likely part of the reason for which we have the synchronization).

Even if somehow we know that there is a guarantee that all calls to line 506 were finished before line 397 is called (seems unlikely, with all those asynchronous calls above---also, if we could enforce such a guarantee, we may have better ways to communicate completion than to enqueue ON_COMPLETE_EVENT in the shared eventsToDeliver queue), it still seems like a fragile guarantee to make, because external callers may not be aware of it (e.g., it is not documented) or about other timing assumptions.

This Fix Code

This fix is very simple: just surround the add() at line 397 with a synchronized (eventsToDeliver), just like the other lines discussed above (373-376, 424-426, 493-494, 493-498, and 493-506).

Testing

mvn clean install

[debora-ito]

The fix was release in SDK version 2.15.20.