[jira] [Resolved] (DIRMINA-779) SSLHandler can re-order data that it reads

["Emmanuel Lecharny (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/DIRMINA-779?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Emmanuel Lecharny resolved DIRMINA-779.
---------------------------------------
    Resolution: Fixed

Using a lock instead of a synchronized (this) does not break the ssltest, and should provide the right protection against concurrent access.

Fixed with commit 56a6e58004ea4a6af5640f03d1f6796a073c5d62

> SSLHandler can re-order data that it reads
> ------------------------------------------
>
>                 Key: DIRMINA-779
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-779
>             Project: MINA
>          Issue Type: Bug
>          Components: Filter
>    Affects Versions: 1.1.7, 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4, 2.0.0-M5, 2.0.0-M6, 2.0.0-RC1
>            Reporter: Jason Resch
>             Fix For: 2.0.8
>
>         Attachments: ssl_reodering_fix.diff
>
>
> The code in question is the flushScheduledEvents() method in SSLHandler.java:
> {code}
>  public void flushScheduledEvents() {
>    // Fire events only when no lock is hold for this handler.
>    if (Thread.holdsLock(this)) {
>        return;
>    }
>    Event e;
>          // We need synchronization here inevitably because filterWrite can be
>    // called simultaneously and cause 'bad record MAC' integrity error.
>    synchronized (this) {
>        while ((e = filterWriteEventQueue.poll()) != null) {
>            e.nextFilter.filterWrite(session, (WriteRequest) e.data);
>        }
>    }
>    while ((e = messageReceivedEventQueue.poll()) != null) {
>        e.nextFilter.messageReceived(session, e.data);
>    }
>  }
> {code}
>  This method is called both by threads which handle writes, and threads that
>  handle reads.  Therefore, as the comments suggest, multiple threads may go
>  through this code simultaneously.  However, since there is no
>  synchronization around processing of the messageReceivedEventQueue, it is
>  possible that the received messages will be sent to the next filter out of
>  order, should there be more than one message in the queue and a context
>  switch happen at the wrong time.
>  The bug would manifest in our application as a failure of our protocol layer
>  to decode a message, we believe, due to a re-ordering.  It only occurred in
>  environments with a large amount of contention and network traffic and when
>  using TLS.  The fix I have tested was to move the closing brace of the
>  synchronized block to extend to cover both while loops.  I've attached a
>  patch representing that change.  Since making that change we have not
>  encountered the bug again after about 30 hours of testing and 1.5 TB of
>  traffic, whereas before the change we could reproduce it after a few
>  minutes.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)