gistfile1.diff

From 8fa980873165ae02fb53317563f9f115e905d5c2 Mon Sep 17 00:00:00 2001
From: Paulo Matias <paulo.matias@usp.br>
Date: Sun, 2 May 2010 15:21:43 -0300
Subject: [PATCH] Read all records to always empty the OpenSSL reading buffer.

---
 lib/net.js         |   26 +++++++++++++++++++++-----
 src/node_crypto.cc |   10 ++++++++++
 src/node_crypto.h  |    1 +
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/lib/net.js b/lib/net.js
index 74659cc..ae8f6f6 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -296,11 +296,27 @@ function initStream (self) {
     try {
       if (self.secure) {
         if (!securePool) allocNewSecurePool();
-        secureBytesRead = read(self.fd, securePool, 0, securePool.length);
-        self.secureStream.readInject(securePool, 0, secureBytesRead);
-        bytesRead = self.secureStream.readExtract(pool,
-                                                  pool.used,
-                                                  pool.length - pool.used);
+        var calledByNextTick = (arguments.length == 0); // IOWatcher always passes arguments
+        if (!calledByNextTick) {
+          secureBytesRead = read(self.fd, securePool, 0, securePool.length);
+          self.secureStream.readInject(securePool, 0, secureBytesRead);
+        }
+        var chunkBytes;
+        bytesRead = 0;
+        do {
+          chunkBytes = self.secureStream.readExtract(pool,
+                                                     pool.used + bytesRead,
+                                                     pool.length - pool.used - bytesRead);
+          bytesRead += chunkBytes;
+        } while ((chunkBytes > 0) && (pool.used + bytesRead < pool.length));
+        if (bytesRead == 0 && calledByNextTick)
+          return;
+        if (self.secureStream.readPending()) {
+          process.nextTick(function () {
+            if(self._readWatcher)
+              self._readWatcher.callback();
+          });
+        }
         if (!self.secureEstablished) {
           if (self.secureStream.isInitFinished()) {
             self.secureEstablished = true;
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 653c759..f925030 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -566,6 +566,8 @@ void SecureStream::Initialize(Handle<Object> target) {
                             SecureStream::WriteInject);
   NODE_SET_PROTOTYPE_METHOD(t, "writeExtract",
                             SecureStream::WriteExtract);
+  NODE_SET_PROTOTYPE_METHOD(t, "readPending",
+                            SecureStream::ReadPending);
   NODE_SET_PROTOTYPE_METHOD(t, "writeCanExtract",
                             SecureStream::WriteCanExtract);
   NODE_SET_PROTOTYPE_METHOD(t, "getPeerCertificate",
@@ -717,6 +719,14 @@ Handle<Value> SecureStream::ReadExtract(const Arguments& args) {
   return scope.Close(Integer::New(bytes_read));
 }
 
+Handle<Value> SecureStream::ReadPending(const Arguments& args) {
+  HandleScope scope;
+
+  SecureStream *ss = ObjectWrap::Unwrap<SecureStream>(args.Holder());
+  int bytes_pending = BIO_pending(ss->pbioRead);
+  return scope.Close(Integer::New(bytes_pending));
+}
+
 Handle<Value> SecureStream::WriteCanExtract(const Arguments& args) {
   HandleScope scope;
 
diff --git a/src/node_crypto.h b/src/node_crypto.h
index 56f5e0e..27790d3 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -51,6 +51,7 @@ class SecureStream : ObjectWrap {
   static v8::Handle<v8::Value> New(const v8::Arguments& args);
   static v8::Handle<v8::Value> ReadInject(const v8::Arguments& args);
   static v8::Handle<v8::Value> ReadExtract(const v8::Arguments& args);
+  static v8::Handle<v8::Value> ReadPending(const v8::Arguments& args);
   static v8::Handle<v8::Value> WriteCanExtract(const v8::Arguments& args);
   static v8::Handle<v8::Value> WriteExtract(const v8::Arguments& args);
   static v8::Handle<v8::Value> WriteInject(const v8::Arguments& args);
-- 
1.6.4.4

The issue is not so simple like I thought when writing the previous patch. OpenSSL may buffer partial records that it may had received across multiple IOWatcher callback calls, causing the buffer to be overflown if we don't take care. Also, OpenSSL may compress data (see [1]), so it's never guaranteed that the pool will have enough room for the unencrypted data.

My approach was to take care to not overflow the pool, then to call the callback again in nextTick if there was pending data after the loop.

Does anybody has a more elegant solution?

[1] http://httpd.apache.org/docs/2.0/ssl/ssl_faq.html#comp