Merge pull request #354 from jruby/gem-error-minimal

SSLSocket write and non-block fixes

Author: kares

Mavenfile

@@ -82,10 +82,13 @@ plugin :clean do
                  'failOnError' =>  'false' )
 end
 
-jar 'org.jruby:jruby-core', '9.2.1.0', :scope => :provided
+jruby_compile_compat = '9.2.1.0'
+jar 'org.jruby:jruby-core', jruby_compile_compat, :scope => :provided
 # for invoker generated classes we need to add javax.annotation when on Java > 8
 jar 'javax.annotation:javax.annotation-api', '1.3.1', :scope => :compile
 jar 'org.junit.jupiter:junit-jupiter', '5.11.4', :scope => :test
+# a test dependency to provide digest and other stdlib bits, needed when loading OpenSSL in Java unit tests
+jar 'org.jruby:jruby-stdlib', jruby_compile_compat, :scope => :test
 
 plugin :surefire, '3.5.5'
 

pom.xml

@@ -113,6 +113,12 @@ DO NOT MODIFY - GENERATED CODE
       <version>5.11.4</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.jruby</groupId>
+      <artifactId>jruby-stdlib</artifactId>
+      <version>9.2.1.0</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
   <repositories>
     <repository>

src/main/java/org/jruby/ext/openssl/SSLSocket.java

@@ -141,14 +141,15 @@ private static CallSite callSite(final CallSite[] sites, final CallSiteIndex ind
         return sites[ index.ordinal() ];
     }
 
-    private SSLContext sslContext;
+    private static final ByteBuffer EMPTY_DATA = ByteBuffer.allocate(0).asReadOnlyBuffer();
+
+    SSLContext sslContext;
     private SSLEngine engine;
     private RubyIO io;
 
-    private ByteBuffer appReadData;
-    private ByteBuffer netReadData;
-    private ByteBuffer netWriteData;
-    private final ByteBuffer dummy = ByteBuffer.allocate(0); // could be static
+    ByteBuffer appReadData;
+    ByteBuffer netReadData;
+    ByteBuffer netWriteData;
 
     private boolean initialHandshake = false;
     private transient long initializeTime;
@@ -209,7 +210,7 @@ private IRubyObject fallback_set_io_nonblock_checked(ThreadContext context, Ruby
 
     private static final String SESSION_SOCKET_ID = "socket_id";
 
-    private SSLEngine ossl_ssl_setup(final ThreadContext context, final boolean server) {
+    SSLEngine ossl_ssl_setup(final ThreadContext context, final boolean server) {
         SSLEngine engine = this.engine;
         if ( engine != null ) return engine;
 
@@ -553,10 +554,6 @@ private static void writeWouldBlock(final Ruby runtime, final boolean exception,
         result[0] = WRITE_WOULD_BLOCK_RESULT;
     }
 
-    private void doHandshake(final boolean blocking) throws IOException {
-        doHandshake(blocking, true);
-    }
-
     // might return :wait_readable | :wait_writable in case (true, false)
     private IRubyObject doHandshake(final boolean blocking, final boolean exception) throws IOException {
         while (true) {
@@ -578,7 +575,7 @@ private IRubyObject doHandshake(final boolean blocking, final boolean exception)
                 doTasks();
                 break;
             case NEED_UNWRAP:
-                if (readAndUnwrap(blocking) == -1 && handshakeStatus != SSLEngineResult.HandshakeStatus.FINISHED) {
+                if (readAndUnwrap(blocking, exception) == -1 && handshakeStatus != SSLEngineResult.HandshakeStatus.FINISHED) {
                     throw new SSLHandshakeException("Socket closed");
                 }
                 // during initialHandshake, calling readAndUnwrap that results UNDERFLOW does not mean writable.
@@ -614,7 +611,7 @@ private IRubyObject doHandshake(final boolean blocking, final boolean exception)
 
     private void doWrap(final boolean blocking) throws IOException {
         netWriteData.clear();
-        SSLEngineResult result = engine.wrap(dummy, netWriteData);
+        SSLEngineResult result = engine.wrap(EMPTY_DATA.duplicate(), netWriteData);
         netWriteData.flip();
         handshakeStatus = result.getHandshakeStatus();
         status = result.getStatus();
@@ -689,7 +686,9 @@ public int write(ByteBuffer src, boolean blocking) throws SSLException, IOExcept
             if ( netWriteData.hasRemaining() ) {
                 flushData(blocking);
             }
-            netWriteData.clear();
+            // compact() to preserve encrypted bytes flushData could not send (non-blocking partial write)
+            // clear() would discard them, corrupting the TLS record stream:
+            netWriteData.compact();
             final SSLEngineResult result = engine.wrap(src, netWriteData);
             if ( result.getStatus() == SSLEngineResult.Status.CLOSED ) {
                 throw getRuntime().newIOError("closed SSL engine");
@@ -704,11 +703,15 @@ public int write(ByteBuffer src, boolean blocking) throws SSLException, IOExcept
     }
 
     public int read(final ByteBuffer dst, final boolean blocking) throws IOException {
+        return read(dst, blocking, true);
+    }
+
+    private int read(final ByteBuffer dst, final boolean blocking, final boolean exception) throws IOException {
         if ( initialHandshake ) return 0;
         if ( engine.isInboundDone() ) return -1;
 
         if ( ! appReadData.hasRemaining() ) {
-            int appBytesProduced = readAndUnwrap(blocking);
+            int appBytesProduced = readAndUnwrap(blocking, exception);
             if (appBytesProduced == -1 || appBytesProduced == 0) {
                 return appBytesProduced;
             }
@@ -719,17 +722,16 @@ public int read(final ByteBuffer dst, final boolean blocking) throws IOException
         return limit;
     }
 
-    private int readAndUnwrap(final boolean blocking) throws IOException {
+    private int readAndUnwrap(final boolean blocking, final boolean exception) throws IOException {
         final int bytesRead = socketChannelImpl().read(netReadData);
         if ( bytesRead == -1 ) {
             if ( ! netReadData.hasRemaining() ||
                  ( status == SSLEngineResult.Status.BUFFER_UNDERFLOW ) ) {
                 closeInbound();
                 return -1;
             }
-            // inbound channel has been already closed but closeInbound() must
-            // be defered till the last engine.unwrap() call.
-            // peerNetData could not be empty.
+            // inbound channel has been already closed but closeInbound() must be defered till
+            // the last engine.unwrap() call; peerNetData could not be empty
         }
         appReadData.clear();
         netReadData.flip();
@@ -768,7 +770,7 @@ private int readAndUnwrap(final boolean blocking) throws IOException {
                 handshakeStatus == SSLEngineResult.HandshakeStatus.NEED_TASK ||
                 handshakeStatus == SSLEngineResult.HandshakeStatus.NEED_WRAP ||
                 handshakeStatus == SSLEngineResult.HandshakeStatus.FINISHED ) ) {
-            doHandshake(blocking);
+            doHandshake(blocking, exception);
         }
         return appReadData.remaining();
     }
@@ -793,7 +795,7 @@ private void doShutdown() throws IOException {
         }
         netWriteData.clear();
         try {
-            engine.wrap(dummy, netWriteData); // send close (after sslEngine.closeOutbound)
+            engine.wrap(EMPTY_DATA.duplicate(), netWriteData); // send close (after sslEngine.closeOutbound)
         }
         catch (SSLException e) {
             debug(getRuntime(), "SSLSocket.doShutdown", e);
@@ -808,10 +810,10 @@ private void doShutdown() throws IOException {
     }
 
     /**
-     * @return the (@link RubyString} buffer or :wait_readable / :wait_writeable {@link RubySymbol}
+     * @return the {@link RubyString} buffer or :wait_readable / :wait_writeable {@link RubySymbol}
      */
-    private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject len, final IRubyObject buff,
-                                    final boolean blocking, final boolean exception) {
+    private IRubyObject sysreadImpl(final ThreadContext context,
+        final IRubyObject len, final IRubyObject buff, final boolean blocking, final boolean exception) {
         final Ruby runtime = context.runtime;
 
         final int length = RubyNumeric.fix2int(len);
@@ -831,6 +833,14 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l
         }
 
         try {
+            // Flush pending write data before reading (after write_nonblock encrypted bytes may still be buffered)
+            if ( engine != null && netWriteData.hasRemaining() ) {
+                if ( flushData(blocking) && ! blocking ) {
+                    if ( exception ) throw newSSLErrorWaitWritable(runtime, "write would block");
+                    return runtime.newSymbol("wait_writable");
+                }
+            }
+
             // So we need to make sure to only block when there is no data left to process
             if ( engine == null || ! ( appReadData.hasRemaining() || netReadData.position() > 0 ) ) {
                 final Object ex = waitSelect(SelectionKey.OP_READ, blocking, exception);
@@ -839,12 +849,12 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l
 
             final ByteBuffer dst = ByteBuffer.allocate(length);
             int read = -1;
-            // ensure >0 bytes read; sysread is blocking read.
+            // ensure > 0 bytes read; sysread is blocking read
             while ( read <= 0 ) {
                 if ( engine == null ) {
                     read = socketChannelImpl().read(dst);
                 } else {
-                    read = read(dst, blocking);
+                    read = read(dst, blocking, exception);
                 }
 
                 if ( read == -1 ) {
@@ -1226,7 +1236,7 @@ public IRubyObject ssl_version(ThreadContext context) {
         return context.runtime.newString( engine.getSession().getProtocol() );
     }
 
-    private transient SocketChannelImpl socketChannel;
+    transient SocketChannelImpl socketChannel;
 
     private SocketChannelImpl socketChannelImpl() {
         if ( socketChannel != null ) return socketChannel;
@@ -1241,7 +1251,7 @@ private SocketChannelImpl socketChannelImpl() {
         throw new IllegalStateException("unknow channel impl: " + channel + " of type " + channel.getClass().getName());
     }
 
-    private interface SocketChannelImpl {
+    interface SocketChannelImpl {
 
         boolean isOpen() ;
 

src/test/java/org/jruby/ext/openssl/OpenSSLHelper.java

@@ -0,0 +1,70 @@
+package org.jruby.ext.openssl;
+
+import org.jruby.Ruby;
+import org.jruby.runtime.ThreadContext;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+abstract class OpenSSLHelper {
+
+    protected Ruby runtime;
+
+    void setUpRuntime() throws ClassNotFoundException {
+        runtime = Ruby.newInstance();
+        loadOpenSSL(runtime);
+    }
+
+    void tearDownRuntime() {
+        if (runtime != null) runtime.tearDown(false);
+    }
+
+    protected void loadOpenSSL(final Ruby runtime) throws ClassNotFoundException {
+        // prepend lib/ so openssl.rb + jopenssl/ are loaded instead of bundled OpenSSL in jruby-stdlib
+        final String libDir = new File("lib").getAbsolutePath();
+        runtime.evalScriptlet("$LOAD_PATH.unshift '" + libDir + "'");
+        runtime.evalScriptlet("require 'openssl'");
+
+        // sanity: verify openssl was loaded from the project, not jruby-stdlib :
+        final String versionFile = new File(libDir, "jopenssl/version.rb").getAbsolutePath();
+        final String expectedVersion = runtime.evalScriptlet(
+                "File.read('" + versionFile + "').match( /.*\\sVERSION\\s*=\\s*['\"](.*)['\"]/ )[1]")
+                .toString();
+        final String loadedVersion = runtime.evalScriptlet("JOpenSSL::VERSION").toString();
+        assertEquals(expectedVersion, loadedVersion, "OpenSSL must be loaded from project " +
+                "(got version " + loadedVersion + "), not from jruby-stdlib");
+
+        // Also check the Java extension classes were resolved from the project, not jruby-stdlib :
+        final String classOrigin = runtime.getJRubyClassLoader()
+                .loadClass("org.jruby.ext.openssl.OpenSSL")
+                .getProtectionDomain().getCodeSource().getLocation().toString();
+        assertTrue(classOrigin.endsWith("/pkg/classes/"), "OpenSSL.class (via JRuby classloader) " +
+                "come from project, got: " + classOrigin);
+    }
+
+    // HELPERS
+
+    public ThreadContext currentContext() {
+        return runtime.getCurrentContext();
+    }
+
+    public static String readResource(final String resource) {
+        int n;
+        try (InputStream in = SSLSocketTest.class.getResourceAsStream(resource)) {
+            if (in == null) throw new IllegalArgumentException(resource + " not found on classpath");
+
+            ByteArrayOutputStream out = new ByteArrayOutputStream();
+            byte[] buf = new byte[8192];
+            while ((n = in.read(buf)) != -1) out.write(buf, 0, n);
+            return new String(out.toByteArray(), StandardCharsets.UTF_8);
+        } catch (IOException e) {
+            throw new IllegalStateException("failed to load" + resource, e);
+        }
+    }
+}