Skip to content
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.

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

Fix crypto abort on throwing setter #27157

Closed

Conversation

sam-github
Copy link
Contributor

See #26868 (comment)

@addaleax Note there are 2 commits. Actually, I'm not convinced the second commit is correct.

If a C++ API called from js has an openssl error occur, then while creating the error object to throw from C++, it calls into user-land js, and THAT throws, which error should actually suface? With all the Maybe checking, I made it be the "pending" low-level error. That was a useful exercise, it gives me a better understanding what all the Maybe's are about, but I have to say, I'm not sure its what I would expect.

In this case, isn't it the first error, the openssl API one, that we would want thrown? Not the second one?

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

@sam-github sam-github requested a review from addaleax April 9, 2019 19:13
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot 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 Apr 9, 2019
@sam-github sam-github changed the title Fix tls abort on throwing setter Fix crypto abort on throwing setter Apr 9, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you!

src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@sam-github sam-github force-pushed the fix-abort-on-throwing-setter branch 2 times, most recently from 9686a81 to cf6e4d2 Compare April 11, 2019 22:28
@sam-github
Copy link
Contributor Author

@addaleax made your suggested changes, still this open question:

Actually, I'm not convinced the second commit is correct.

If a C++ API called from js has an openssl error occur, then while creating the error object to throw from C++, it calls into user-land js, and THAT throws, which error should actually suface? With all the Maybe checking, I made it be the "pending" low-level error. That was a useful exercise, it gives me a better understanding what all the Maybe's are about, but I have to say, I'm not sure its what I would expect.

In this case, isn't it the first error, the openssl API one, that we would want thrown? Not the second one?

@addaleax
Copy link
Member

@sam-github Yeah, I don’t think there’s a definite right or wrong answer. But I think forwarding the setter exception is what would happen if we were to do this in JS land (unless there are explicit try/catchs), and it might also be odd to throw an “incomplete” OpenSSL error object otherwise?

@sam-github
Copy link
Contributor Author

@addaleax the analogy with js is not entirely perfect, because the openssl error is not (yet) an exception, but it is an "error". The properties are doced as "exist if they do" (lots of crypto errors don't come from openssl, so don't have the properties), so that isn't a problem, but I'll leave it as it is, throwing the first pending exception. Its a total fringe case, its hard to imagine anyone attaching throwing setters on the global object prototype and expecting deterministic behaviour.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor Author

Landed in 69140bc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants