[ 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: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org