Skip to content

Crypto add opensslerror stack #15518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gla5001 wants to merge 8 commits into from
Closed

Crypto add opensslerror stack #15518

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f2c9e49
crypto: better crypto error messages
gla5001 Aug 10, 2017
dc2165a
Review updates and tests
gla5001 Aug 11, 2017
5c24f07
Additional review updates.
gla5001 Aug 15, 2017
d1296cf
Review update. Use the overload of Set()
gla5001 Aug 15, 2017
2b177cb
Nit fix: changing local variable to snake_case
gla5001 Aug 16, 2017
bbaea8b
PR review updates
gla5001 Sep 21, 2017
a5a2f64
changing openSSLErrorStack property name to opensslErrorStack
gla5001 Sep 22, 2017
2ad7440
fixing long doc line
gla5001 Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Additional review updates.
  • Loading branch information
@gla5001
gla5001 committed Sep 28, 2017
commit 5c24f07ab7f9081a6069d39e2e3c124d2adf4443
7 changes: 4 additions & 3 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,10 @@ intercepted.
A generic JavaScript `Error` object that does not denote any specific
circumstance of why the error occurred. `Error` objects capture a "stack trace"
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, if it's available when the error is thrown, in
a separate property.
provide a text description of the error.
Copy link
Copy Markdown
Contributor Author

@gla5001 gla5001 Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit has the latest review comment fixes.
@bnoordhuis, i believe i've addressed all of your comments. Thanks for the review.


For crypto only, `Error` objects will include the OpenSSL error stack in a
separate property 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.
Expand Down
1 change: 1 addition & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ class ModuleWrap;
V(onstreamclose_string, "onstreamclose") \
V(ontrailers_string, "ontrailers") \
V(onwrite_string, "onwrite") \
V(openssl_error_stack, "openSSLErrorStack") \
V(output_string, "output") \
V(order_string, "order") \
V(owner_string, "owner") \
Expand Down
15 changes: 6 additions & 9 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,20 +262,16 @@ void ThrowCryptoError(Environment* env,
ERR_error_string_n(err, errmsg, sizeof(errmsg));
message = String::NewFromUtf8(env->isolate(), errmsg,
v8::NewStringType::kNormal)
.ToLocalChecked();
.ToLocalChecked();
} else {
message = String::NewFromUtf8(env->isolate(), default_message,
v8::NewStringType::kNormal)
.ToLocalChecked();
.ToLocalChecked();
}

Local<Value> exception_v = Exception::Error(message);
CHECK(!exception_v.IsEmpty());
// Add the openSSLErrorStack property to the exception object.
Local<Object> exception = exception_v.As<Object>();
Local<String> key = String::NewFromUtf8(env->isolate(), "openSSLErrorStack",
v8::NewStringType::kInternalized)
.ToLocalChecked();
Local<Array> errorStack = Array::New(env->isolate());

ERR_STATE *es = ERR_get_state();
Expand All @@ -289,17 +285,18 @@ void ThrowCryptoError(Environment* env,
ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr));
errorStack->Set(i, String::NewFromUtf8(env->isolate(), tmpStr,
v8::NewStringType::kNormal)
.ToLocalChecked());
.ToLocalChecked());
}
Copy link
Copy Markdown
Contributor Author

@gla5001 gla5001 Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle the ring buffer with modular arithmetic

es->top -= 1;
}

// Adding the new property that will look like the following:
// 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->context(), key, errorStack).FromJust();
exception->Set(env->openssl_error_stack(), errorStack);
env->isolate()->ThrowException(exception);
}

Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-crypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ assert.throws(function() {
if ((err instanceof Error) &&
/digest too big for rsa key$/.test(err) &&
err.openSSLErrorStack !== undefined &&
Array.isArray(err.openSSLErrorStack) &&
err.openSSLErrorStack.length === 0) {
return true;
Copy link
Copy Markdown
Contributor Author

@gla5001 gla5001 Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update test since the openSSLErrorStack is not always added

}
Expand All @@ -265,6 +266,7 @@ assert.throws(function() {
if ((err instanceof Error) &&
/asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/.test(err) &&
err.openSSLErrorStack !== undefined &&
Array.isArray(err.openSSLErrorStack) &&
err.openSSLErrorStack.length > 0) {
return true;
}
Expand Down