Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Won't Fix
-
None
-
None
-
None
-
None
Description
This class is within the 'Apache James :: Server :: Data :: File Persistence' project.
In the list() method, there is a line of code that potentially invokes undefined behavior. Around line 577, the construction of keys can invoke undefined behavior because the ArrayList constructor that is invoked iterates over the given collection (see https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#ArrayList-java.util.Collection-), and the documentation for Hashtable.keySet() states:
If the map is modified while an iteration over the set is in progress (except through the iterator's own remove operation), the results of the iteration are undefined.
In the case where the mList Hashtable is modified while another thread is within list() making a copy of the key set, this invokes undefined behavior.
One way of fixing this particular issue is to manually synchronize on mList while keys is being constructed (although, this is technically not covered in the documentation, so the fix of manually synching on mList would be relying on the specifics of a particular implementation; it would be guaranteed if Collections.synchronizedMap() were used instead).
However, there are other thread safety issues in the form of unsynchronized access to the mList and mboxFile fields. Some threads might be setting these fields to different values while other threads are trying to read them.
To illustrate my concern, consider the following scenario: One thread is within selectMessage() around line 415. The thread retrieves the current value of field mList and finds that the current reference is not null. Right at that moment, another thread within findMessage() sets mList to null. Then the first thread retrieves mList again (now null) in order to call containsKey(), but because mList is null, a NullPointerException is thrown.
This hypothetical scenario will probably not happen in real life because the first thread will probably retrieve the value of the mList field only once (see https://stackoverflow.com/q/32996785/196844). Nevertheless, I believe that the Java Language Specification does not require an implementation to retrieve the value of the field only once.