An `std::string_view v` is a `const char* v.data()` along with an `std::size_t v.size()` that guarantees that `v.size()` contiguous elements of type `char` can be accessed relative to the pointer `v.data()`. One of the main reasons behind the existence of `std::string_view` is the ability to operate on `char` sequences without requiring null termination, which otherwise often requires expensive copies of strings to be made. As a consequence, it is generally incorrect to assume that `v.data()` points to a null-terminated sequence of `char`, and the only way to obtain a null-terminated string from an `std::string_view` is to make a copy. It is not even possible to check if the sequence pointed to by `v.data()` is null-terminated because the null character would be at position `v.data() + v.size()`, which is outside of the range that `v` guarantees safe access to. (A default-constructed `std::string_view` even sets its own data pointer to a `nullptr`, which is fine because it only needs to guarantee safe access to zero elements, i.e., to no elements). In `deps/ncrypto` and `src/crypto`, there are various APIs that consume `std::string_view v` arguments but then ignore `v.size()` and treat `v.data()` as a C-style string of type `const char*`. However, that is not what call sites would expect from functions that explicitly ask for `std::string_view` arguments, since it makes assumptions beyond the guarantees provided by `std::string_view` and leads to undefined behavior unless the given view either contains an embedded null character or the `char` at address `v.data() + v.size()` is a null character. This is not a reasonable assumption for `std::string_view` in general, and it also defeats the purpose of `std::string_view` for the most part since, when `v.size()` is being ignored, it is essentially just a `const char*`. Constructing an `std::string_view` from a `const char*` is also not "free" but requires computing the length of the C-style string (unless the length can be computed at compile time, e.g., because the value is just a string literal). Repeated conversion between `const char*` as used by OpenSSL and `std::string_view` as used by ncrypto thus incurs the additional overhead of computing the length of the string whenever an `std::string_view` is constructed from a `const char*`. (This seems negligible compared to the safety argument though.) Similarly, returning a `const char*` pointer to a C-style string as an `std::string_view` has two downsides: the function must compute the length of the string in order to construct the view, and the caller can no longer assume that the return value is null-terminated and thus cannot pass the returned view to functions that require their arguments to be null terminated. (And, for the reasons explained above, the caller also cannot check if the value is null-terminated without potentially invoking undefined behavior.) C++20 unfortunately does not have a type similar to Rust's `CStr` or GSL `czstring`. Therefore, this commit changes many occurrences of `std::string_view` back to `const char*`, which is conventional for null-terminated C-style strings and does not require computing the length of strings. There are _a lot_ of occurrences of `std::string_view` in ncrypto and for each one, we need to evaluate if it is safe and a good abstraction. I tried to do so, but I might have changed too few or too many, so please feel free to give feedback on individual occurrences. PR-URL: https://github.com/nodejs/node/pull/57816 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
94 lines
2.5 KiB
C++
94 lines
2.5 KiB
C++
#include "ncrypto.h"
|
|
|
|
namespace ncrypto {
|
|
|
|
// ============================================================================
|
|
// Engine
|
|
|
|
#ifndef OPENSSL_NO_ENGINE
|
|
EnginePointer::EnginePointer(ENGINE* engine_, bool finish_on_exit_)
|
|
: engine(engine_), finish_on_exit(finish_on_exit_) {}
|
|
|
|
EnginePointer::EnginePointer(EnginePointer&& other) noexcept
|
|
: engine(other.engine), finish_on_exit(other.finish_on_exit) {
|
|
other.release();
|
|
}
|
|
|
|
EnginePointer::~EnginePointer() {
|
|
reset();
|
|
}
|
|
|
|
EnginePointer& EnginePointer::operator=(EnginePointer&& other) noexcept {
|
|
if (this == &other) return *this;
|
|
this->~EnginePointer();
|
|
return *new (this) EnginePointer(std::move(other));
|
|
}
|
|
|
|
void EnginePointer::reset(ENGINE* engine_, bool finish_on_exit_) {
|
|
if (engine != nullptr) {
|
|
if (finish_on_exit) {
|
|
// This also does the equivalent of ENGINE_free.
|
|
ENGINE_finish(engine);
|
|
} else {
|
|
ENGINE_free(engine);
|
|
}
|
|
}
|
|
engine = engine_;
|
|
finish_on_exit = finish_on_exit_;
|
|
}
|
|
|
|
ENGINE* EnginePointer::release() {
|
|
ENGINE* ret = engine;
|
|
engine = nullptr;
|
|
finish_on_exit = false;
|
|
return ret;
|
|
}
|
|
|
|
EnginePointer EnginePointer::getEngineByName(const char* name,
|
|
CryptoErrorList* errors) {
|
|
MarkPopErrorOnReturn mark_pop_error_on_return(errors);
|
|
EnginePointer engine(ENGINE_by_id(name));
|
|
if (!engine) {
|
|
// Engine not found, try loading dynamically.
|
|
engine = EnginePointer(ENGINE_by_id("dynamic"));
|
|
if (engine) {
|
|
if (!ENGINE_ctrl_cmd_string(engine.get(), "SO_PATH", name, 0) ||
|
|
!ENGINE_ctrl_cmd_string(engine.get(), "LOAD", nullptr, 0)) {
|
|
engine.reset();
|
|
}
|
|
}
|
|
}
|
|
return engine;
|
|
}
|
|
|
|
bool EnginePointer::setAsDefault(uint32_t flags, CryptoErrorList* errors) {
|
|
if (engine == nullptr) return false;
|
|
ClearErrorOnReturn clear_error_on_return(errors);
|
|
return ENGINE_set_default(engine, flags) != 0;
|
|
}
|
|
|
|
bool EnginePointer::init(bool finish_on_exit) {
|
|
if (engine == nullptr) return false;
|
|
if (finish_on_exit) setFinishOnExit();
|
|
return ENGINE_init(engine) == 1;
|
|
}
|
|
|
|
EVPKeyPointer EnginePointer::loadPrivateKey(const char* key_name) {
|
|
if (engine == nullptr) return EVPKeyPointer();
|
|
return EVPKeyPointer(
|
|
ENGINE_load_private_key(engine, key_name, nullptr, nullptr));
|
|
}
|
|
|
|
void EnginePointer::initEnginesOnce() {
|
|
static bool initialized = false;
|
|
if (!initialized) {
|
|
ENGINE_load_builtin_engines();
|
|
ENGINE_register_all_complete();
|
|
initialized = true;
|
|
}
|
|
}
|
|
|
|
#endif // OPENSSL_NO_ENGINE
|
|
|
|
} // namespace ncrypto
|