Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quic/decrypt_crypto: Reuse OpenSSL context objects #14

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

awelzel
Copy link

@awelzel awelzel commented Oct 12, 2023

It is not necessary to allocate and free the context objects used for HKDF and AES all the time, they can be re-used. The main assumption here is no cross-thread usage, but this should be guaranteed even with the fibers: QUIC_decrypt_crypto_payload() always runs to completion.

A pcap with ~12k QUIC connections had ~15% samples in QUIC_decrypt_crypto_payload. After this change it is down to 5% of samples. The improvement in runtime is ~16%, 12.2 seconds to 10.2 seconds.


This is lots of copy paste currently, I have the skills to do #define, but wonder if there might be a nicer approch.

@awelzel awelzel requested a review from bbannier October 12, 2023 16:48
Copy link
Member

@bbannier bbannier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code clearly works today, but it will fail in very subtle ways (in production) should we ever process analyzers in parallel, e.g., during DPD. I feel this needs a very very fat warning on the top explaining why this is not thread safe at all.

static EVP_CIPHER_CTX* ctx = nullptr;
if ( ! ctx )
{
ctx = EVP_CIPHER_CTX_new();
Copy link
Member

@bbannier bbannier Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a note, in multi-threaded code this would not be safe since ctx might be set once you reach this assignment. You'd either need to use double-checked locking, or use a static stack variable which is guaranteed by the language to be initialized exactly once on first use, e.g.,

EVP_CIPHER_CTX* get_aes_128_ecb()
	{
	static struct Ctx
		{
		Ctx() : val(EVP_CIPHER_CTX_new())
			{
			EVP_CipherInit_ex(val, EVP_aes_128_ecb(), NULL, NULL, NULL, 1);
			}
		EVP_CIPHER_CTX* val;
		} ctx;

	return ctx.val;
	}

Using such a struct would actually give you the option of implementing a destructor which calls EVP_PKEY_CTX_free so you do not leak the context anymore (which might work if the order in which static objects are destructed works out right).

analyzer/decrypt_crypto.cc Show resolved Hide resolved
It is not necessary to allocate and free the context objects used for
HKDF and AES all the time, they can be re-used. The main assumption here
is no cross-thread usage, but this should be guaranteed even with the
fibers: QUIC_decrypt_crypto_payload() always runs to completion.

A pcap with ~12k QUIC connections had ~15% samples in
QUIC_decrypt_crypto_payload. After this change it is down to 5%
of samples. The improvement in runtime is ~16%, 12.2 seconds
to 10.2 seconds.
@awelzel awelzel force-pushed the topic/awelzel/reuse-openssl-ctx-objects branch from 7da20f9 to 46ae055 Compare October 12, 2023 18:19
@awelzel awelzel merged commit 06b6b3d into main Oct 12, 2023
3 checks passed
@awelzel awelzel deleted the topic/awelzel/reuse-openssl-ctx-objects branch October 12, 2023 18:28
awelzel added a commit to zeek/zeek that referenced this pull request Oct 12, 2023
It is not necessary to allocate and free the context objects used for
HKDF and AES all the time, they can be re-used. The main assumption here
is no cross-thread usage, but this should be guaranteed even with the
fibers: QUIC_decrypt_crypto_payload() always runs to completion.

A pcap with ~12k QUIC connections had ~15% samples in
QUIC_decrypt_crypto_payload. After this change it is down to 5%
of samples. The improvement in runtime is ~16%, 12.2 seconds
to 10.2 seconds.

From zeek/spicy-quic#14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants