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:

{quote}
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.
{quote}

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
(v6.4.14#64029)

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


Mime
View raw message