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