james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Trebbien (JIRA)" <server-...@james.apache.org>
Subject [jira] [Created] (JAMES-2200) Potential undefined behavior in org.apache.james.mailrepository.file.MBoxMailRepository
Date Fri, 20 Oct 2017 23:18:00 GMT
Daniel Trebbien created JAMES-2200:

             Summary: Potential undefined behavior in org.apache.james.mailrepository.file.MBoxMailRepository
                 Key: JAMES-2200
                 URL: https://issues.apache.org/jira/browse/JAMES-2200
             Project: James Server
          Issue Type: Bug
            Reporter: Daniel Trebbien

_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|https://github.com/apache/james-project/blob/bc995fc1e8324ef5adfbf3de329546a8e42125fb/server/data/data-file/src/main/java/org/apache/james/mailrepository/file/MBoxMailRepository.java#L577],
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|https://github.com/apache/james-project/blob/bc995fc1e8324ef5adfbf3de329546a8e42125fb/server/data/data-file/src/main/java/org/apache/james/mailrepository/file/MBoxMailRepository.java#L415].
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.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org

View raw message