[
https://issues.apache.org/jira/browse/JAMES-2200?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Trebbien updated JAMES-2200:
-----------------------------------
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|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()|https://docs.oracle.com/javase/8/docs/api/java/util/Hashtable.html#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.
was:
_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.
> 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()|https://docs.oracle.com/javase/8/docs/api/java/util/Hashtable.html#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: [email protected]
For additional commands, e-mail: [email protected]
