src: test and code review updates [squash]

nodejs · Jul 12, 2024 · b4bbcab · b4bbcab
1 parent 067c59f
commit b4bbcab
Show file tree

Hide file tree

Showing 7 changed files with 16 additions and 17 deletions.
diff --git a/Makefile b/Makefile
@@ -175,7 +175,7 @@ with-code-cache test-code-cache:
 out/Makefile: config.gypi common.gypi node.gyp \
 	deps/uv/uv.gyp deps/llhttp/llhttp.gyp deps/zlib/zlib.gyp \
 	deps/simdutf/simdutf.gyp deps/ada/ada.gyp deps/nbytes/nbytes.gyp \
-	deps/ncrypto/ncrypto.gyp tools/v8_gypfiles/toolchain.gypi \
+	tools/v8_gypfiles/toolchain.gypi \
 	tools/v8_gypfiles/features.gypi \
 	tools/v8_gypfiles/inspector.gypi tools/v8_gypfiles/v8.gyp
 	$(PYTHON) tools/gyp_node.py -f make

diff --git a/deps/ncrypto/engine.cc b/deps/ncrypto/engine.cc
@@ -28,9 +28,9 @@ void EnginePointer::reset(ENGINE* engine_, bool finish_on_exit_) {
   if (engine != nullptr) {
     if (finish_on_exit) {
       // This also does the equivalent of ENGINE_free.
-      NCRYPTO_ASSERT_EQUAL(ENGINE_finish(engine), 1, "ENGINE_finish");
+      ENGINE_finish(engine);
     } else {
-      NCRYPTO_ASSERT_EQUAL(ENGINE_free(engine), 1, "ENGINE_finish");
+      ENGINE_free(engine);
     }
   }
   engine = engine_;
@@ -70,7 +70,7 @@ bool EnginePointer::setAsDefault(uint32_t flags, CryptoErrorList* errors) {
 bool EnginePointer::init(bool finish_on_exit) {
   if (engine == nullptr) return false;
   if (finish_on_exit) setFinishOnExit();
-  return ENGINE_init(engine) != 0;
+  return ENGINE_init(engine) == 1;
 }
 
 EVPKeyPointer EnginePointer::loadPrivateKey(const std::string_view key_name) {

diff --git a/deps/ncrypto/ncrypto.cc b/deps/ncrypto/ncrypto.cc
@@ -1,5 +1,6 @@
 #include "ncrypto.h"
 #include <algorithm>
+#include <cstring>
 #include "openssl/bn.h"
 #if OPENSSL_VERSION_MAJOR >= 3
 #include "openssl/provider.h"

diff --git a/node.gyp b/node.gyp
@@ -845,8 +845,8 @@
         'deps/simdutf/simdutf.gyp:simdutf',
         'deps/ada/ada.gyp:ada',
         'deps/nbytes/nbytes.gyp:nbytes',
-        'deps/ncrypto/ncrypto.gyp:ncrypto',
         'node_js2c#host',
+        'deps/ncrypto/ncrypto.gyp:ncrypto',
       ],
 
       'sources': [
@@ -1369,9 +1369,9 @@
         'deps/sqlite/sqlite.gyp:sqlite',
         'deps/ada/ada.gyp:ada',
         'deps/nbytes/nbytes.gyp:nbytes',
-        'deps/ncrypto/ncrypto.gyp:ncrypto',
         'deps/simdjson/simdjson.gyp:simdjson',
         'deps/simdutf/simdutf.gyp:simdutf',
+        'deps/ncrypto/ncrypto.gyp:ncrypto',
       ],
 
       'includes': [

diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
@@ -671,7 +671,7 @@ void SecureContext::SetEngineKey(const FunctionCallbackInfo<Value>& args) {
     return;
   }
 
-  if (engine.init(true /* finish on exit*/)) {
+  if (!engine.init(true /* finish on exit*/)) {
     return THROW_ERR_CRYPTO_OPERATION_FAILED(
         env, "Failure to initialize engine");
   }

diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc
@@ -243,7 +243,7 @@ bool CryptoErrorStore::Empty() const {
   return errors_.empty();
 }
 
-v8::MaybeLocal<v8::Value> cryptoErrorListToException(
+MaybeLocal<Value> cryptoErrorListToException(
     Environment* env, const ncrypto::CryptoErrorList& errors) {
   // The CryptoErrorList contains a listing of zero or more errors.
   // If there are no errors, it is likely a bug but we will return
@@ -261,7 +261,7 @@ v8::MaybeLocal<v8::Value> cryptoErrorListToException(
   if (!String::NewFromUtf8(
            env->isolate(), last.data(), NewStringType::kNormal, last.size())
            .ToLocal(&message)) {
-    return MaybeLocal<Value>();
+    return {};
   }
 
   Local<Value> exception = Exception::Error(message);
@@ -277,13 +277,9 @@ v8::MaybeLocal<v8::Value> cryptoErrorListToException(
     auto last = errors.end();
     last--;
     while (current != last) {
-      Local<String> error;
-      if (!String::NewFromUtf8(env->isolate(),
-                               current->data(),
-                               NewStringType::kNormal,
-                               current->size())
-               .ToLocal(&error)) {
-        return MaybeLocal<Value>();
+      Local<Value> error;
+      if (!ToV8Value(env->context(), *current).ToLocal(&error)) {
+        return {};
       }
       stack.push_back(error);
       ++current;
@@ -295,7 +291,7 @@ v8::MaybeLocal<v8::Value> cryptoErrorListToException(
     if (!exception_obj
              ->Set(env->context(), env->openssl_error_stack(), stackArray)
              .IsNothing()) {
-      return MaybeLocal<Value>();
+      return {};
     }
   }
   return exception;

diff --git a/test/parallel/test-process-versions.js b/test/parallel/test-process-versions.js
@@ -34,6 +34,7 @@ if (hasUndici) {
 
 if (common.hasCrypto) {
   expected_keys.push('openssl');
+  expected_keys.push('ncrypto');
 }
 
 if (common.hasQuic) {
@@ -78,6 +79,7 @@ assert.match(process.versions.modules, /^\d+$/);
 assert.match(process.versions.cjs_module_lexer, commonTemplate);
 
 if (common.hasCrypto) {
+  assert.match(process.versions.ncrypto, commonTemplate);
   if (process.config.variables.node_shared_openssl) {
     assert.ok(process.versions.openssl);
   } else {