Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 931ecfa

Browse filesBrowse files
tniessendanielleadams
tniessen
authored and
danielleadams
committed
src: fix memory leaks and refactor ByteSource
Add ByteSource::Builder to replace the common MallocOpenSSL() + ByteSource::Allocated() pattern. Remove ByteSource::reset() that is unused. Remove ByteSource::Resize() to make ByteSource truly immutable (until moved away). Instead, ByteSource::Builder::release() takes an optional size argument that truncates the resulting ByteSource. Fix occurrences of MallocOpenSSL() that do not always free the allocated memory by using the new ByteSource::Builder class instead. Remove ByteSource::get() and replace uses with ByteSource::data(). Remove ReallocOpenSSL() because it likely only saves us a few bytes whenever we use it. PR-URL: #43202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 7e8a00a commit 931ecfa
Copy full SHA for 931ecfa
Expand file treeCollapse file tree

16 files changed

+255
-335
lines changed
Open diff view settings
Collapse file

‎src/crypto/crypto_aes.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_aes.cc
+43-52Lines changed: 43 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,10 @@ WebCryptoCipherStatus AES_Cipher(
8989
case kWebCryptoCipherDecrypt:
9090
// If in decrypt mode, the auth tag must be set in the params.tag.
9191
CHECK(params.tag);
92-
if (!EVP_CIPHER_CTX_ctrl(
93-
ctx.get(),
94-
EVP_CTRL_AEAD_SET_TAG,
95-
params.tag.size(),
96-
const_cast<char*>(params.tag.get()))) {
92+
if (!EVP_CIPHER_CTX_ctrl(ctx.get(),
93+
EVP_CTRL_AEAD_SET_TAG,
94+
params.tag.size(),
95+
const_cast<char*>(params.tag.data<char>()))) {
9796
return WebCryptoCipherStatus::FAILED;
9897
}
9998
break;
@@ -125,9 +124,7 @@ WebCryptoCipherStatus AES_Cipher(
125124
return WebCryptoCipherStatus::FAILED;
126125
}
127126

128-
char* data = MallocOpenSSL<char>(buf_len);
129-
ByteSource buf = ByteSource::Allocated(data, buf_len);
130-
unsigned char* ptr = reinterpret_cast<unsigned char*>(data);
127+
ByteSource::Builder buf(buf_len);
131128

132129
// In some outdated version of OpenSSL (e.g.
133130
// ubi81_sharedlibs_openssl111fips_x64) may be used in sharedlib mode, the
@@ -139,36 +136,36 @@ WebCryptoCipherStatus AES_Cipher(
139136
// Refs: https://github.com/nodejs/node/pull/38913#issuecomment-866505244
140137
if (in.size() == 0) {
141138
out_len = 0;
142-
} else if (!EVP_CipherUpdate(
143-
ctx.get(),
144-
ptr,
145-
&out_len,
146-
in.data<unsigned char>(),
147-
in.size())) {
139+
} else if (!EVP_CipherUpdate(ctx.get(),
140+
buf.data<unsigned char>(),
141+
&out_len,
142+
in.data<unsigned char>(),
143+
in.size())) {
148144
return WebCryptoCipherStatus::FAILED;
149145
}
150146

151147
total += out_len;
152148
CHECK_LE(out_len, buf_len);
153-
ptr += out_len;
154149
out_len = EVP_CIPHER_CTX_block_size(ctx.get());
155-
if (!EVP_CipherFinal_ex(ctx.get(), ptr, &out_len)) {
150+
if (!EVP_CipherFinal_ex(
151+
ctx.get(), buf.data<unsigned char>() + total, &out_len)) {
156152
return WebCryptoCipherStatus::FAILED;
157153
}
158154
total += out_len;
159155

160156
// If using AES_GCM, grab the generated auth tag and append
161157
// it to the end of the ciphertext.
162158
if (cipher_mode == kWebCryptoCipherEncrypt && mode == EVP_CIPH_GCM_MODE) {
163-
data += out_len;
164-
if (!EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_GET_TAG, tag_len, ptr))
159+
if (!EVP_CIPHER_CTX_ctrl(ctx.get(),
160+
EVP_CTRL_AEAD_GET_TAG,
161+
tag_len,
162+
buf.data<unsigned char>() + total))
165163
return WebCryptoCipherStatus::FAILED;
166164
total += tag_len;
167165
}
168166

169167
// It's possible that we haven't used the full allocated space. Size down.
170-
buf.Resize(total);
171-
*out = std::move(buf);
168+
*out = std::move(buf).release(total);
172169

173170
return WebCryptoCipherStatus::OK;
174171
}
@@ -295,38 +292,34 @@ WebCryptoCipherStatus AES_CTR_Cipher(
295292
return WebCryptoCipherStatus::FAILED;
296293
}
297294

298-
// Output size is identical to the input size
299-
char* data = MallocOpenSSL<char>(in.size());
300-
ByteSource buf = ByteSource::Allocated(data, in.size());
301-
unsigned char* ptr = reinterpret_cast<unsigned char*>(data);
295+
// Output size is identical to the input size.
296+
ByteSource::Builder buf(in.size());
302297

303298
// Also just like in chromium's implementation, if we can process
304299
// the input without wrapping the counter, we'll do it as a single
305300
// call here. If we can't, we'll fallback to the a two-step approach
306301
if (BN_cmp(remaining_until_reset.get(), num_output.get()) >= 0) {
307-
auto status = AES_CTR_Cipher2(
308-
key_data,
309-
cipher_mode,
310-
params,
311-
in,
312-
params.iv.data<unsigned char>(),
313-
ptr);
314-
if (status == WebCryptoCipherStatus::OK)
315-
*out = std::move(buf);
302+
auto status = AES_CTR_Cipher2(key_data,
303+
cipher_mode,
304+
params,
305+
in,
306+
params.iv.data<unsigned char>(),
307+
buf.data<unsigned char>());
308+
if (status == WebCryptoCipherStatus::OK) *out = std::move(buf).release();
316309
return status;
317310
}
318311

319312
BN_ULONG blocks_part1 = BN_get_word(remaining_until_reset.get());
320313
BN_ULONG input_size_part1 = blocks_part1 * kAesBlockSize;
321314

322315
// Encrypt the first part...
323-
auto status = AES_CTR_Cipher2(
324-
key_data,
325-
cipher_mode,
326-
params,
327-
ByteSource::Foreign(in.get(), input_size_part1),
328-
params.iv.data<unsigned char>(),
329-
ptr);
316+
auto status =
317+
AES_CTR_Cipher2(key_data,
318+
cipher_mode,
319+
params,
320+
ByteSource::Foreign(in.data<char>(), input_size_part1),
321+
params.iv.data<unsigned char>(),
322+
buf.data<unsigned char>());
330323

331324
if (status != WebCryptoCipherStatus::OK)
332325
return status;
@@ -335,18 +328,16 @@ WebCryptoCipherStatus AES_CTR_Cipher(
335328
std::vector<unsigned char> new_counter_block = BlockWithZeroedCounter(params);
336329

337330
// Encrypt the second part...
338-
status = AES_CTR_Cipher2(
339-
key_data,
340-
cipher_mode,
341-
params,
342-
ByteSource::Foreign(
343-
in.get() + input_size_part1,
344-
in.size() - input_size_part1),
345-
new_counter_block.data(),
346-
ptr + input_size_part1);
347-
348-
if (status == WebCryptoCipherStatus::OK)
349-
*out = std::move(buf);
331+
status =
332+
AES_CTR_Cipher2(key_data,
333+
cipher_mode,
334+
params,
335+
ByteSource::Foreign(in.data<char>() + input_size_part1,
336+
in.size() - input_size_part1),
337+
new_counter_block.data(),
338+
buf.data<unsigned char>() + input_size_part1);
339+
340+
if (status == WebCryptoCipherStatus::OK) *out = std::move(buf).release();
350341

351342
return status;
352343
}
Collapse file

‎src/crypto/crypto_common.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_common.cc
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -525,8 +525,7 @@ MaybeLocal<Value> GetSerialNumber(Environment* env, X509* cert) {
525525
if (bn) {
526526
char* data = BN_bn2hex(bn.get());
527527
ByteSource buf = ByteSource::Allocated(data, strlen(data));
528-
if (buf)
529-
return OneByteString(env->isolate(), buf.get());
528+
if (buf) return OneByteString(env->isolate(), buf.data<unsigned char>());
530529
}
531530
}
532531

Collapse file

‎src/crypto/crypto_dh.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_dh.cc
+4-9Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -606,18 +606,13 @@ ByteSource StatelessDiffieHellmanThreadsafe(
606606
EVP_PKEY_derive(ctx.get(), nullptr, &out_size) <= 0)
607607
return ByteSource();
608608

609-
char* buf = MallocOpenSSL<char>(out_size);
610-
ByteSource out = ByteSource::Allocated(buf, out_size);
611-
612-
if (EVP_PKEY_derive(
613-
ctx.get(),
614-
reinterpret_cast<unsigned char*>(buf),
615-
&out_size) <= 0) {
609+
ByteSource::Builder out(out_size);
610+
if (EVP_PKEY_derive(ctx.get(), out.data<unsigned char>(), &out_size) <= 0) {
616611
return ByteSource();
617612
}
618613

619-
ZeroPadDiffieHellmanSecret(out_size, buf, out.size());
620-
return out;
614+
ZeroPadDiffieHellmanSecret(out_size, out.data<char>(), out.size());
615+
return std::move(out).release();
621616
}
622617
} // namespace
623618

Collapse file

‎src/crypto/crypto_ec.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_ec.cc
+31-51Lines changed: 31 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -486,12 +486,9 @@ Maybe<bool> ECDHBitsTraits::AdditionalConfig(
486486
return Just(true);
487487
}
488488

489-
bool ECDHBitsTraits::DeriveBits(
490-
Environment* env,
491-
const ECDHBitsConfig& params,
492-
ByteSource* out) {
493-
494-
char* data = nullptr;
489+
bool ECDHBitsTraits::DeriveBits(Environment* env,
490+
const ECDHBitsConfig& params,
491+
ByteSource* out) {
495492
size_t len = 0;
496493
ManagedEVPPKey m_privkey = params.private_->GetAsymmetricKey();
497494
ManagedEVPPKey m_pubkey = params.public_->GetAsymmetricKey();
@@ -513,15 +510,14 @@ bool ECDHBitsTraits::DeriveBits(
513510
return false;
514511
}
515512

516-
data = MallocOpenSSL<char>(len);
513+
ByteSource::Builder buf(len);
517514

518-
if (EVP_PKEY_derive(
519-
ctx.get(),
520-
reinterpret_cast<unsigned char*>(data),
521-
&len) <= 0) {
515+
if (EVP_PKEY_derive(ctx.get(), buf.data<unsigned char>(), &len) <= 0) {
522516
return false;
523517
}
524518

519+
*out = std::move(buf).release(len);
520+
525521
break;
526522
}
527523
default: {
@@ -543,22 +539,18 @@ bool ECDHBitsTraits::DeriveBits(
543539
const EC_POINT* pub = EC_KEY_get0_public_key(public_key);
544540
int field_size = EC_GROUP_get_degree(group);
545541
len = (field_size + 7) / 8;
546-
data = MallocOpenSSL<char>(len);
547-
CHECK_NOT_NULL(data);
542+
ByteSource::Builder buf(len);
548543
CHECK_NOT_NULL(pub);
549544
CHECK_NOT_NULL(private_key);
550-
if (ECDH_compute_key(
551-
data,
552-
len,
553-
pub,
554-
private_key,
555-
nullptr) <= 0) {
545+
if (ECDH_compute_key(buf.data<char>(), len, pub, private_key, nullptr) <=
546+
0) {
556547
return false;
557548
}
549+
550+
*out = std::move(buf).release();
558551
}
559552
}
560-
ByteSource buf = ByteSource::Allocated(data, len);
561-
*out = std::move(buf);
553+
562554
return true;
563555
}
564556

@@ -646,7 +638,6 @@ WebCryptoKeyExportStatus EC_Raw_Export(
646638

647639
const EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(m_pkey.get());
648640

649-
unsigned char* data;
650641
size_t len = 0;
651642

652643
if (ec_key == nullptr) {
@@ -666,9 +657,10 @@ WebCryptoKeyExportStatus EC_Raw_Export(
666657
// Get the size of the raw key data
667658
if (fn(m_pkey.get(), nullptr, &len) == 0)
668659
return WebCryptoKeyExportStatus::INVALID_KEY_TYPE;
669-
data = MallocOpenSSL<unsigned char>(len);
670-
if (fn(m_pkey.get(), data, &len) == 0)
660+
ByteSource::Builder data(len);
661+
if (fn(m_pkey.get(), data.data<unsigned char>(), &len) == 0)
671662
return WebCryptoKeyExportStatus::INVALID_KEY_TYPE;
663+
*out = std::move(data).release(len);
672664
} else {
673665
if (key_data->GetKeyType() != kKeyTypePublic)
674666
return WebCryptoKeyExportStatus::INVALID_KEY_TYPE;
@@ -680,17 +672,16 @@ WebCryptoKeyExportStatus EC_Raw_Export(
680672
len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr);
681673
if (len == 0)
682674
return WebCryptoKeyExportStatus::FAILED;
683-
data = MallocOpenSSL<unsigned char>(len);
684-
size_t check_len =
685-
EC_POINT_point2oct(group, point, form, data, len, nullptr);
675+
ByteSource::Builder data(len);
676+
size_t check_len = EC_POINT_point2oct(
677+
group, point, form, data.data<unsigned char>(), len, nullptr);
686678
if (check_len == 0)
687679
return WebCryptoKeyExportStatus::FAILED;
688680

689681
CHECK_EQ(len, check_len);
682+
*out = std::move(data).release();
690683
}
691684

692-
*out = ByteSource::Allocated(reinterpret_cast<char*>(data), len);
693-
694685
return WebCryptoKeyExportStatus::OK;
695686
}
696687
} // namespace
@@ -853,38 +844,27 @@ Maybe<bool> ExportJWKEdKey(
853844
if (!EVP_PKEY_get_raw_public_key(pkey.get(), nullptr, &len))
854845
return Nothing<bool>();
855846

856-
unsigned char* data = MallocOpenSSL<unsigned char>(len);
857-
ByteSource out = ByteSource::Allocated(reinterpret_cast<char*>(data), len);
847+
ByteSource::Builder out(len);
858848

859849
if (key->GetKeyType() == kKeyTypePrivate) {
860-
if (!EVP_PKEY_get_raw_private_key(pkey.get(), data, &len) ||
850+
if (!EVP_PKEY_get_raw_private_key(
851+
pkey.get(), out.data<unsigned char>(), &len) ||
861852
!StringBytes::Encode(
862-
env->isolate(),
863-
reinterpret_cast<const char*>(data),
864-
len,
865-
BASE64URL,
866-
&error).ToLocal(&encoded) ||
867-
!target->Set(
868-
env->context(),
869-
env->jwk_d_string(),
870-
encoded).IsJust()) {
853+
env->isolate(), out.data<const char>(), len, BASE64URL, &error)
854+
.ToLocal(&encoded) ||
855+
!target->Set(env->context(), env->jwk_d_string(), encoded).IsJust()) {
871856
if (!error.IsEmpty())
872857
env->isolate()->ThrowException(error);
873858
return Nothing<bool>();
874859
}
875860
}
876861

877-
if (!EVP_PKEY_get_raw_public_key(pkey.get(), data, &len) ||
862+
if (!EVP_PKEY_get_raw_public_key(
863+
pkey.get(), out.data<unsigned char>(), &len) ||
878864
!StringBytes::Encode(
879-
env->isolate(),
880-
reinterpret_cast<const char*>(data),
881-
len,
882-
BASE64URL,
883-
&error).ToLocal(&encoded) ||
884-
!target->Set(
885-
env->context(),
886-
env->jwk_x_string(),
887-
encoded).IsJust()) {
865+
env->isolate(), out.data<const char>(), len, BASE64URL, &error)
866+
.ToLocal(&encoded) ||
867+
!target->Set(env->context(), env->jwk_x_string(), encoded).IsJust()) {
888868
if (!error.IsEmpty())
889869
env->isolate()->ThrowException(error);
890870
return Nothing<bool>();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.