Crypto add opensslerror stack by gla5001 · Pull Request #15518 · nodejs/node · GitHub

Feature request to add openSSL error stack to the exception object
thrown from crypto. New exception property only added to object
if the error stack has not cleared out prior to calling
ThrowCryptoError.

I did something very wrong when trying to rebase, so i just created a new branch and a new PR. This PR has all the changes requested from #14725. I will close the other one.

Refs: #5444

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto

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

Now, we wont add the property to the exception every time.

handle the ring buffer with modular arithmetic

this is not needed

update test since the openSSLErrorStack is not always added

The only new commit is 720a5af. Everything else was reviewed in #14725.

ping @nodejs/crypto

long line here?

LGTM

CI https://ci.nodejs.org/job/node-test-pull-request/10293/

@gla5001 when trying to rebase this on the CI it gets a conflict. I am a bit surprised that this is not shown here but would you be so kind and rebase this nevertheless?

@BridgeAR sure thing

Feature request to add openSSL error stack to the exception object thrown from crypto. New exception property only added to object if the error stack has not cleared out prior to calling ThrowCryptoError. Refs: nodejs#5444

@BridgeAR rebased and pushed. Could you let me know if this resolves the issue?

CI https://ci.nodejs.org/job/node-test-pull-request/10302/

Landed in ccfcd88

Add openSSL error stack to the exception object thrown from crypto. The new exception property is only added to the object if the error stack has not cleared out prior to calling ThrowCryptoError. PR-URL: #15518 Refs: #5444 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

Add openSSL error stack to the exception object thrown from crypto. The new exception property is only added to the object if the error stack has not cleared out prior to calling ThrowCryptoError. PR-URL: nodejs#15518 Refs: nodejs#5444 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

The commit log could have had a link to #14725, that's where 95% of the review took place. This comment will have to do.

Add openSSL error stack to the exception object thrown from crypto. The new exception property is only added to the object if the error stack has not cleared out prior to calling ThrowCryptoError. PR-URL: #15518 Refs: #5444 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

Add openSSL error stack to the exception object thrown from crypto. The new exception property is only added to the object if the error stack has not cleared out prior to calling ThrowCryptoError. PR-URL: nodejs/node#15518 Refs: nodejs/node#5444 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

Add openSSL error stack to the exception object thrown from crypto. The new exception property is only added to the object if the error stack has not cleared out prior to calling ThrowCryptoError. PR-URL: #15518 Refs: #5444 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

edit: this likely shouldn'y be backported if it is changing error messages prior to error codes... but I wanted to confirm

@MylesBorins I believe it should not be backported.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Sep 21, 2017

gla5001 commented Sep 21, 2017

View reviewed changes

gla5001 mentioned this pull request Sep 21, 2017

crypto: better crypto error messages #14725

Closed

4 tasks

jasnell requested review from bnoordhuis and targos September 21, 2017 12:59

sam-github approved these changes Sep 22, 2017

View reviewed changes

jasnell reviewed Sep 25, 2017

View reviewed changes

indutny approved these changes Sep 26, 2017

View reviewed changes

gla5001 added 8 commits September 27, 2017 21:48

crypto: better crypto error messages

f2c9e49

Feature request to add openSSL error stack to the exception object thrown from crypto. New exception property only added to object if the error stack has not cleared out prior to calling ThrowCryptoError. Refs: nodejs#5444

Review updates and tests

dc2165a

Additional review updates.

5c24f07

Review update. Use the overload of Set()

d1296cf

Nit fix: changing local variable to snake_case

2b177cb

PR review updates

bbaea8b

changing openSSLErrorStack property name to opensslErrorStack

a5a2f64

fixing long doc line

2ad7440

gla5001 force-pushed the crypto-add-opensslerror-stack branch from 537284e to 2ad7440 Compare September 28, 2017 03:16

BridgeAR closed this Sep 28, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the backport-requested-v6.x label Oct 17, 2017

MylesBorins added dont-land-on-v6.x and removed backport-requested-v6.x labels Oct 17, 2017

bnoordhuis mentioned this pull request Oct 26, 2017

Can we refactor opensslErrorStack? #16521

Closed

-                    Array.isArray(err.openSSLErrorStack) &&
-                    err.openSSLErrorStack.length === 0) {
+                    err.openSSLErrorStack === undefined) {
                   return true;

		@@ -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.

+                if (es->bottom != es->top) {
+                  Local<Array> error_stack = Array::New(env->isolate());
+                  int top = es->top;

+                    // Using ERR_NUM_ERRORS  macro defined in openssl.
+                    es->top = (((es->top - 1) % ERR_NUM_ERRORS) + ERR_NUM_ERRORS) %
+                        ERR_NUM_ERRORS;
                   }


		ERR_set_mark();

		bp = BIO_new_mem_buf(const_cast<char*>(key_pem), key_pem_len);

Crypto add opensslerror stack by gla5001 · Pull Request #15518 · nodejs/node · GitHub

Uh oh!

Uh oh!

Crypto add opensslerror stack - #15518

Crypto add opensslerror stack#15518gla5001 wants to merge 8 commits intonodejs:masternodejs/node:masterfrom gla5001:crypto-add-opensslerror-stackgla5001/node:crypto-add-opensslerror-stackCopy head branch name to clipboard

Conversation

gla5001 commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

gla5001 Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gla5001 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

gla5001 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

gla5001 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

gla5001 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

gla5001 commented Sep 21, 2017

Uh oh!

jasnell commented Sep 21, 2017

Uh oh!

jasnell Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Sep 27, 2017

Uh oh!

gla5001 commented Sep 28, 2017

Uh oh!

gla5001 commented Sep 28, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

bnoordhuis commented Sep 28, 2017

Uh oh!

MylesBorins commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gla5001 commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Crypto add opensslerror stack#15518
gla5001 wants to merge 8 commits into
nodejs:masterfrom
gla5001:crypto-add-opensslerror-stack

gla5001 commented Sep 21, 2017 •
edited
Loading

gla5001 Sep 21, 2017 •
edited
Loading

BridgeAR commented Sep 27, 2017 •
edited
Loading

MylesBorins commented Oct 17, 2017 •
edited
Loading