Skip to content

src: fix error handling for CryptoJob::ToResult

The added test case crashes in recent versions of Node.js 15 due to a change in OpenSSL and improper error handling in node core.

The first problem is that ManagedEVPPKey::ToEncodedPublicKey and ManagedEVPPKey::ToEncodedPrivateKey returned Just(false) to indicate that an exception was thrown, but KeyPairGenTraits::EncodeKey only checks IsNothing() and not FromJust(). This leads to JavaScript handles being empty, and, consequently, to a crash in V8.

The next problem is that some code paths in ToResult throw exceptions whereas others leave error handling to CryptoJob::AfterThreadPoolWork. In the test case added here, the function WritePublicKey throws an exception because the curve does not have an OID.

I am not entirely sure why these functions use Maybe<bool> as their return type. Overall, I am pretty sure we need to rework substantial parts of the error handling logic in the crypto subsystem. Hopefully, this PR can still provide a reasonable workaround to avoid crashes for now.

I tried to keep the existing behavior of ignoring any results that are equal to Just(false), meaning that returning Just(false) from ToResult causes the operation to never complete or fail.

When ToResult returns Nothing<bool>(), the AfterThreadPoolWork function now assumes that an exception was thrown, and passes it to the job callback.

In synchronous mode, both Just(false) and Nothing<bool>() are ignored, which leads to exceptions being propagated to JavaScript correctly.

@jasnell I'd love to hear your opinion. I am happy to implement and test another solution. We should clarify the semantics of the tristate Maybe<bool>.

Merge request reports

Loading