You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2015/10/26 16:10:53 UTC

svn commit: r1710618 - in /tomcat/trunk/java/org/apache/tomcat/util/net: SecureNio2Channel.java openssl/OpenSSLEngine.java

Author: remm
Date: Mon Oct 26 15:10:53 2015
New Revision: 1710618

URL: http://svn.apache.org/viewvc?rev=1710618&view=rev
Log:
- Fix capacity check algorithm (overflow isn't the right result in that case).
- Unwrap first in NIO2 and wait for an explicit underflow to read on the socket (I'll test adding a flag to optimize this since this is likely a bit expensive, but commit it for now since everything's now working).

Modified:
    tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java
    tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java

Modified: tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java?rev=1710618&r1=1710617&r2=1710618&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java Mon Oct 26 15:10:53 2015
@@ -572,14 +572,9 @@ public class SecureNio2Channel extends N
 
     private class FutureRead implements Future<Integer> {
         private final ByteBuffer dst;
-        private final Future<Integer> integer;
+        private final Future<Integer> integer = null;
         private FutureRead(ByteBuffer dst) {
             this.dst = dst;
-            if (netInBuffer.position() > 0) {
-                this.integer = null;
-            } else {
-                this.integer = sc.read(netInBuffer);
-            }
         }
         @Override
         public boolean cancel(boolean mayInterruptIfRunning) {
@@ -837,11 +832,7 @@ public class SecureNio2Channel extends N
                 handler.failed(exc, attach);
             }
         };
-        if (netInBuffer.position() > 0) {
-            readCompletionHandler.completed(Integer.valueOf(netInBuffer.position()), attachment);
-        } else {
-            sc.read(netInBuffer, timeout, unit, attachment, readCompletionHandler);
-        }
+        readCompletionHandler.completed(Integer.valueOf(netInBuffer.position()), attachment);
     }
 
     @Override

Modified: tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java?rev=1710618&r1=1710617&r2=1710618&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java Mon Oct 26 15:10:53 2015
@@ -571,13 +571,12 @@ public final class OpenSSLEngine extends
         int pendingApp = (handshakeFinished || SSL.isInInit(ssl) == 0) ? SSL.pendingReadableBytesInSSL(ssl) : 0;
         int bytesProduced = 0;
         int idx = offset;
+        // Do we have enough room in dsts to write decrypted data?
+        if (capacity < pendingApp) {
+            return new SSLEngineResult(SSLEngineResult.Status.BUFFER_OVERFLOW, getHandshakeStatus(), bytesConsumed, 0);
+        }
 
         while (pendingApp > 0) {
-            // Do we have enough room in dsts to write decrypted data?
-            if (capacity < pendingApp) {
-                return new SSLEngineResult(SSLEngineResult.Status.BUFFER_OVERFLOW, getHandshakeStatus(), bytesConsumed, 0);
-            }
-
             // Write decrypted data to dsts buffers
             while (idx < endOffset) {
                 ByteBuffer dst = dsts[idx];
@@ -612,6 +611,8 @@ public final class OpenSSLEngine extends
             if (pendingApp == 0) {
                 primingSSLRead();
                 pendingApp = SSL.pendingReadableBytesInSSL(ssl);
+            } else if (capacity == 0) {
+                break;
             }
         }
 



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


Re: svn commit: r1710618 - in /tomcat/trunk/java/org/apache/tomcat/util/net: SecureNio2Channel.java openssl/OpenSSLEngine.java

Posted by Rémy Maucherat <re...@apache.org>.
2015-10-26 20:50 GMT+01:00 Mark Thomas <ma...@apache.org>:

> On 26/10/2015 08:10, remm@apache.org wrote:
> > Author: remm
> > Date: Mon Oct 26 15:10:53 2015
> > New Revision: 1710618
> >
> > URL: http://svn.apache.org/viewvc?rev=1710618&view=rev
> > Log:
> > - Fix capacity check algorithm (overflow isn't the right result in that
> case).
> > - Unwrap first in NIO2 and wait for an explicit underflow to read on the
> socket (I'll test adding a flag to optimize this since this is likely a bit
> expensive, but commit it for now since everything's now working).
>
> Just to clarify, do you mean that all the various issues you were
> observing with NIO and NIO2 with the OpenSSL engine with and without h2
> are now resolved? If so, WOOT!
>
> It looks good for me with Chrome and HTTP/2. I can't say if everything is
fine yet, but it's likely. The behavior you found is listed as a known bug
in OpenSSL's API, unfortunately the native code didn't work around it. So
at least the engine has to do it since the behavior is obviously not
consistent with JSSE's engine.

NIO and its non blocking reads behaved better in most cases, it would read
0 and proceed with unwrapping (again), while NIO2 would not unwrap again
until it got more data [which isn't bad by itself, but this doesn't work
due to the OpenSSL's bug].

Rémy

Re: svn commit: r1710618 - in /tomcat/trunk/java/org/apache/tomcat/util/net: SecureNio2Channel.java openssl/OpenSSLEngine.java

Posted by Mark Thomas <ma...@apache.org>.
On 26/10/2015 08:10, remm@apache.org wrote:
> Author: remm
> Date: Mon Oct 26 15:10:53 2015
> New Revision: 1710618
> 
> URL: http://svn.apache.org/viewvc?rev=1710618&view=rev
> Log:
> - Fix capacity check algorithm (overflow isn't the right result in that case).
> - Unwrap first in NIO2 and wait for an explicit underflow to read on the socket (I'll test adding a flag to optimize this since this is likely a bit expensive, but commit it for now since everything's now working).

Just to clarify, do you mean that all the various issues you were
observing with NIO and NIO2 with the OpenSSL engine with and without h2
are now resolved? If so, WOOT!

Mark

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