[
https://issues.apache.org/jira/browse/FTPSERVER-33?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Niklas Gustavsson closed FTPSERVER-33.
--------------------------------------
Resolution: Invalid
Assignee: Niklas Gustavsson
Very old issue, no longer valid
> Race conditions in Apache FTP Server
> ------------------------------------
>
> Key: FTPSERVER-33
> URL: https://issues.apache.org/jira/browse/FTPSERVER-33
> Project: FtpServer
> Issue Type: Bug
> Environment: Founded by Mayur Naik by his static race detection tool.
> Reporter: Sergey Vladimirov
> Assignee: Niklas Gustavsson
> Priority: Minor
> Attachments: FTPSERVER-33a.txt, FTPSERVER-33b.txt
>
>
> Cutting from letter to ftpserver-dev
> "I'm a PhD student in Computer Science at Stanford University, evaluating a
> static race detection tool I've developed for Java, on open source programs.
> I
> ran the tool on Apache FTP Server's source code and found a bunch of races,
> see:
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/
> Since this is a static tool, the race reports aren't concrete test cases.
> Moreover, some will be false positives. I've inspected most of them myself
> and
> I'll summarize possible bugs below:
> Bug #1:
> There are race reports on field m_maxIdleTimeSec of class BaseUser and on
> field
> m_request of class RequestHandler, see for instance:
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/2_trace.html
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/8_trace.html
> These races indicate a possible bug. In particular, the server thread calls
> method newConnection(IConnection) of class ConnectionManagerImpl, which
> executes the code:
> cons.add(connection);
> ...
> BaseUser user = (BaseUser)connection.getRequest().getUser();
> user.setMaxIdleTime(m_defaultIdleSec);
> Notice that it sets m_maxIdleTimeSec *after* adding the connection to
> m_conList, which is incorrect because the timer thread may fire while the
> server thread is executing the "..." code above. The timer thread will sweep
> over m_conList, find this connection which was just added, and execute the
> following code:
> FtpRequestImpl request = (FtpRequestImpl)con.getRequest();
> if (request.isTimeout(currTime)) { *** }
> where the isTimeout method calls getMaxIdleTime() (hence the race on field
> m_maxIdleTimeSec), which may return some value of m_maxIdleTimeSec other than
> m_defaultIdleSec which the server thread wanted to set after executing the
> "..." code above.
> Likewise, the race on field m_request suggests that the server thread may get
> a
> null pointer exception because it executes getRequest() above and does not
> check if the value returned is non-null. In particular, the timer thread may
> close the connection that was just added to m_conList by the main server
> thread
> (note that the close() method of class RequestHandler sets field m_request of
> the connection to null, hence the race on field m_request).
> Of course, the above scenarios may be *highly* improbable in practice, but
> since my tool is static, it just reports possible races -- not whether or how
> probable they are in actual runs. It's easy to fix the races merely by moving
> the code that adds the connection to m_consList in method
> newConnection(IConnection) to after the code that sets its max idle time.
> Bug #2:
> You might want to declare field m_isConnectionClosed of class RequestHandler
> 'volatile'. See for instance this race report:
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/15_trace.html
> In the absence of the volatile keyword, under the latest Java Memory Model, it
> is legal for the unsynchronized accesses to this field in the run() method of
> class RequestHandler to return a stale value (ex. from the local cache of a
> processor on a multi-processor machine). In particular, even if the timer
> thread sets this field to true for a connection it has closed, the connection
> thread executing the run() method may still see it's value as false. The way
> to fix this problem is to use synchronization in the run() method (see related
> Bug #3 below) or to declare the field m_isConnectionClosed as volatile.
> Declaring it volatile ensures that the latest value of the field will be read.
> Bug #3:
> There are several races on fields like m_request, m_reader, and m_writer, and
> m_controlSocket, see for instance:
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/27_trace.html
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/24_trace.html
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/16_trace.html
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/19_trace.html
> They occur because the run() method of class RequestHandler may be executed by
> a connection thread concurrently with the close() method of class
> RequestHandler by a timer thread. The run() method reads these fields without
> synchronization and dereferences them without checking for non-nullness, while
> the close() method sets them to null. Have you considered the possibility of
> a
> null pointer exception happening because of the above scenario? I ask because
> the run() method has a loop that repeatedly dereferences these fields.
> Perhaps
> you don't expect this scenario to ever occur, but if you do expect it to
> occur,
> then you need to put the body of the entire run() method or at least the body
> of the loop in it inside a block synchronized on 'this'.
> Bug #4:
> Finally, there are races on several fields of class FtpStatisticsImpl (ex.
> m_currConnections, m_totalConnections, m_currLogins, m_totalLogins, etc., see
> for instance:
> http://suif.stanford.edu/~mhn/ftpserver-dev/target/classes/results/38_trace.html
> There is no synchronization in this class although its fields may be
> incremented and decremented by different threads concurrently. Is this
> intentional, for performance reasons?"
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
