PR review updates

nodejs · gla5001 · Aug 10, 2017 · Aug 11, 2017 · Aug 15, 2017 · Aug 15, 2017
commit bbaea8b461bef9e8ae59dcb96b3c0b85a886f866
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -193,7 +193,7 @@ detailing the point in the code at which the `Error` was instantiated, and may
 provide a text description of the error.
 
 For crypto only, `Error` objects will include the OpenSSL error stack in a
-separate property if it is available when the error is thrown.
+separate property called `openSSLErrorStack` if it is available when the error is thrown.
 
 All errors generated by Node.js, including all System and JavaScript errors,
 will either be instances of, or inherit from, the `Error` class.

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -272,32 +272,47 @@ void ThrowCryptoError(Environment* env,
   Local<Value> exception_v = Exception::Error(message);
   CHECK(!exception_v.IsEmpty());
   Local<Object> exception = exception_v.As<Object>();
-  Local<Array> error_stack = Array::New(env->isolate());
-
-  ERR_STATE *es = ERR_get_state();
-  // Build the error_stack array to be added to openSSLErrorStack property.
-  for (unsigned int i = 0; es->bottom != es->top
-        && (es->err_flags[es->top] & ERR_FLAG_MARK) == 0; i++) {
-    unsigned long err_buf = es->err_buffer[es->top];  // NOLINT(runtime/int)
-    // Only add error string if there is valid err_buffer.
-    if (err_buf) {
-      char tmpStr[256] = { };
-      ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr));
-      error_stack->Set(env->context(), i,
-                      String::NewFromUtf8(env->isolate(), tmpStr,
-                                             v8::NewStringType::kNormal)
-                                                .ToLocalChecked()).FromJust();
+  ERR_STATE* es = ERR_get_state();
+
+  if (es->bottom != es->top) {
+    Local<Array> error_stack = Array::New(env->isolate());
+    int top = es->top;
+
+    // Build the error_stack array to be added to openSSLErrorStack property.
+    for (unsigned int i = 0; es->bottom != es->top;) {
+      unsigned long err_buf = es->err_buffer[es->top];  // NOLINT(runtime/int)
+      // Only add error string if there is valid err_buffer.
+      if (err_buf) {
+        char tmp_str[256];
+        ERR_error_string_n(err_buf, tmp_str, sizeof(tmp_str));
+        error_stack->Set(env->context(), i,
+                        String::NewFromUtf8(env->isolate(), tmp_str,
+                                              v8::NewStringType::kNormal)
+                                                  .ToLocalChecked()).FromJust();
+        // Only increment if we added to error_stack.
+        i++;
+      }
+
+      // Since the ERR_STATE is a ring buffer, we need to use modular
+      // arithmetic to loop back around in the case where bottom is after top.
+      // Using ERR_NUM_ERRORS  macro defined in openssl.
+      es->top = (((es->top - 1) % ERR_NUM_ERRORS) + ERR_NUM_ERRORS) %
+          ERR_NUM_ERRORS;
     }
-    es->top -= 1;
+
+    // Restore top.
+    es->top = top;
+
+    // Add the openSSLErrorStack property to the exception object.
+    // The new property will look like the following:
+    // openSSLErrorStack: [
+    // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
+    // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 err'
+    // ]
+    exception->Set(env->context(), env->openssl_error_stack(), error_stack)
+        .FromJust();
   }
 
-  // Add the openSSLErrorStack property to the exception object.
-  // The new property will look like the following:
-  // openSSLErrorStack: [
-  // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
-  // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error'
-  // ]
-  exception->Set(env->openssl_error_stack(), error_stack);
   env->isolate()->ThrowException(exception);
 }
 
@@ -4315,8 +4330,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
   if (!initialised_)
     return kSignNotInitialised;
 
-  // ClearErrorOnReturn clear_error_on_return;
-
   EVP_PKEY* pkey = nullptr;
   BIO* bp = nullptr;
   X509* x509 = nullptr;
@@ -4326,8 +4339,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
   int r = 0;
   EVP_PKEY_CTX* pkctx = nullptr;
 
-  ERR_set_mark();
-
   bp = BIO_new_mem_buf(const_cast<char*>(key_pem), key_pem_len);
   if (bp == nullptr)
     goto exit;

diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js
@@ -235,13 +235,9 @@ assert.throws(function() {
   ].join('\n');
   crypto.createSign('SHA256').update('test').sign(priv);
 }, (err) => {
-  // Throws crypto error, so there is an openSSLErrorStack property.
-  // The openSSL stack is empty.
   if ((err instanceof Error) &&
       /digest too big for rsa key$/.test(err) &&
-      err.openSSLErrorStack !== undefined &&
-      Array.isArray(err.openSSLErrorStack) &&
-      err.openSSLErrorStack.length === 0) {
+      err.openSSLErrorStack === undefined) {
     return true;
   }
 });