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>
Previously we have been using the variant of SnapshotCreator that
only passes the external references instead of
v8::Isolate::CreateParams and it's about to be deprecated.
Switch to using the new constructor that takes a fully CreateParams
instead.
This also makes sure that the snapshot building script is using
the Node.js array buffer allocator instead of a separate default
one that was previously used by the old constructor. The zero fill
toggle in the Node.js array buffer allocator would still be ignored
during snapshot building, however, until we fixes the array buffer
allocator and let V8 own the toggle backing store instead, because
otherwise the snapshot would contain the external toggle address
and become unreproducible.
PR-URL: https://github.com/nodejs/node/pull/55337
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
The fourth positional argument of `createCipherBase` is `true` when
called from within the `Cipheriv` constructor and `false`when called
from within the `Decipheriv` constructor. This value is then passed to
the `CipherBase::New()` method, which treats it as follows:
args[0]->IsTrue() ? kCipher : kDecipher
However, the current name of said positional argument is `decipher` and
thus indicates the exact opposite: when the argument is set to true, the
instance is *not* a `Decipheriv` object. Therefore, this commit renames
the argument to `isEncrypt`, which matches the actual semantics.
PR-URL: https://github.com/nodejs/node/pull/57843
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
All util.types.is##Type() calls are ported to the fast API.
This improves the performance for multiple APIs such as:
- util.inspect (and util.format and console methods respectively)
- util.isDeepStrictEqual
- assert.(not)deepEqual (including strict and partial mode)
It will also improve any other API where these APIs are used.
PR-URL: https://github.com/nodejs/node/pull/57819
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The `CipherBase` class assumes that any authentication tag will fit into
`EVP_GCM_TLS_TAG_LEN` bytes, which is true because Node.js only supports
GCM with AES as the blocker cipher, and the block size of AES happens to
be 16 bytes, which coincidentally is also the output size of the
Poly1305 construction used by ChaCha20-Poly1305 as well as the maximum
size of authentication tags produced by AES in CCM or OCB mode.
This commit adds a new constant `ncrypto::Cipher::MAX_AUTH_TAG_LENGTH`
which is the maximum length of authentication tags produced by
algorithms that Node.js supports and replaces some constants in
`CipherBase` with semantically more meaningful named constants.
The OpenSSL team is debating whether a constant like
`MAX_AUTH_TAG_LENGTH` (`EVP_MAX_AEAD_TAG_LENGTH`) should exist at all
since its value necessarily depends on the set of AEAD algorithms
supported, but I do believe that, for Node.js, this is a step in the
right direction. It certainly makes more sense than to use the AES-GCM
tag size as defined by TLS.
PR-URL: https://github.com/nodejs/node/pull/57803
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
clarify the example presented where multiple REPL
instances are run in the same process by:
- making its title unambiguous
- clarifying that they share the same `global` object
(which currently is a bit ambiguous/half implied)
- making sure that they do share the same `global`
object (via `useGlobal` set to `true`)
- they delete the unix socket if present
(so that people running the code twice don't get
confused/annoyed that the second time it doesn't
properly work)
PR-URL: https://github.com/nodejs/node/pull/57759
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see https://github.com/google/brotli/issues/1159
PR-URL: https://github.com/nodejs/node/pull/57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
The current examples presented at the bottom
of the REPL doc have two issues:
- they look like they're part of the section
above (on how to run multiple REPL instances
in the same process) but they are not
- the alert informing readers not to use the
second example in production environments
can be wrongly interpreted as to refer to
both examples
The changes here address both these issues
PR-URL: https://github.com/nodejs/node/pull/57762
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Gürgün Dayıoğlu