Skip to content

Commit 720a5af

Browse files
committed
PR review updates
1 parent c035ba0 commit 720a5af

File tree

3 files changed

+40
-33
lines changed

3 files changed

+40
-33
lines changed

doc/api/errors.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ detailing the point in the code at which the `Error` was instantiated, and may
193193
provide a text description of the error.
194194

195195
For crypto only, `Error` objects will include the OpenSSL error stack in a
196-
separate property if it is available when the error is thrown.
196+
separate property called `openSSLErrorStack` if it is available when the error is thrown.
197197

198198
All errors generated by Node.js, including all System and JavaScript errors,
199199
will either be instances of, or inherit from, the `Error` class.

src/node_crypto.cc

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -272,32 +272,47 @@ void ThrowCryptoError(Environment* env,
272272
Local<Value> exception_v = Exception::Error(message);
273273
CHECK(!exception_v.IsEmpty());
274274
Local<Object> exception = exception_v.As<Object>();
275-
Local<Array> error_stack = Array::New(env->isolate());
276-
277-
ERR_STATE *es = ERR_get_state();
278-
// Build the error_stack array to be added to openSSLErrorStack property.
279-
for (unsigned int i = 0; es->bottom != es->top
280-
&& (es->err_flags[es->top] & ERR_FLAG_MARK) == 0; i++) {
281-
unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int)
282-
// Only add error string if there is valid err_buffer.
283-
if (err_buf) {
284-
char tmpStr[256] = { };
285-
ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr));
286-
error_stack->Set(env->context(), i,
287-
String::NewFromUtf8(env->isolate(), tmpStr,
288-
v8::NewStringType::kNormal)
289-
.ToLocalChecked()).FromJust();
275+
ERR_STATE* es = ERR_get_state();
276+
277+
if (es->bottom != es->top) {
278+
Local<Array> error_stack = Array::New(env->isolate());
279+
int top = es->top;
280+
281+
// Build the error_stack array to be added to openSSLErrorStack property.
282+
for (unsigned int i = 0; es->bottom != es->top;) {
283+
unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int)
284+
// Only add error string if there is valid err_buffer.
285+
if (err_buf) {
286+
char tmp_str[256];
287+
ERR_error_string_n(err_buf, tmp_str, sizeof(tmp_str));
288+
error_stack->Set(env->context(), i,
289+
String::NewFromUtf8(env->isolate(), tmp_str,
290+
v8::NewStringType::kNormal)
291+
.ToLocalChecked()).FromJust();
292+
// Only increment if we added to error_stack.
293+
i++;
294+
}
295+
296+
// Since the ERR_STATE is a ring buffer, we need to use modular
297+
// arithmetic to loop back around in the case where bottom is after top.
298+
// Using ERR_NUM_ERRORS macro defined in openssl.
299+
es->top = (((es->top - 1) % ERR_NUM_ERRORS) + ERR_NUM_ERRORS) %
300+
ERR_NUM_ERRORS;
290301
}
291-
es->top -= 1;
302+
303+
// Restore top.
304+
es->top = top;
305+
306+
// Add the openSSLErrorStack property to the exception object.
307+
// The new property will look like the following:
308+
// openSSLErrorStack: [
309+
// 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
310+
// 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 err'
311+
// ]
312+
exception->Set(env->context(), env->openssl_error_stack(), error_stack)
313+
.FromJust();
292314
}
293315

294-
// Add the openSSLErrorStack property to the exception object.
295-
// The new property will look like the following:
296-
// openSSLErrorStack: [
297-
// 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
298-
// 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error'
299-
// ]
300-
exception->Set(env->openssl_error_stack(), error_stack);
301316
env->isolate()->ThrowException(exception);
302317
}
303318

@@ -4315,8 +4330,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
43154330
if (!initialised_)
43164331
return kSignNotInitialised;
43174332

4318-
// ClearErrorOnReturn clear_error_on_return;
4319-
43204333
EVP_PKEY* pkey = nullptr;
43214334
BIO* bp = nullptr;
43224335
X509* x509 = nullptr;
@@ -4326,8 +4339,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
43264339
int r = 0;
43274340
EVP_PKEY_CTX* pkctx = nullptr;
43284341

4329-
ERR_set_mark();
4330-
43314342
bp = BIO_new_mem_buf(const_cast<char*>(key_pem), key_pem_len);
43324343
if (bp == nullptr)
43334344
goto exit;

test/parallel/test-crypto.js

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -235,13 +235,9 @@ assert.throws(function() {
235235
].join('\n');
236236
crypto.createSign('SHA256').update('test').sign(priv);
237237
}, (err) => {
238-
// Throws crypto error, so there is an openSSLErrorStack property.
239-
// The openSSL stack is empty.
240238
if ((err instanceof Error) &&
241239
/digest too big for rsa key$/.test(err) &&
242-
err.openSSLErrorStack !== undefined &&
243-
Array.isArray(err.openSSLErrorStack) &&
244-
err.openSSLErrorStack.length === 0) {
240+
err.openSSLErrorStack === undefined) {
245241
return true;
246242
}
247243
});

0 commit comments

Comments
 (0)