-
Notifications
You must be signed in to change notification settings - Fork 178
fix(asio): memory leaks over ssl::stream lifetime (IDFGH-16977) #978
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
Open
rtyle
wants to merge
2
commits into
espressif:master
Choose a base branch
from
rtyle:fix/asio/memory-leaks-over-ssl-stream-lifetime
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
fix(asio): memory leaks over ssl::stream lifetime (IDFGH-16977) #978
rtyle
wants to merge
2
commits into
espressif:master
from
rtyle:fix/asio/memory-leaks-over-ssl-stream-lifetime
+30
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asio::ssl::stream leaks a significant amount (>20KB) of memory over its
lifetime.
asio::ssl::mbedtls::engine::impl has these members ...
mbedtls_ssl_context ssl_{};
mbedtls_entropy_context entropy_{};
mbedtls_ctr_drbg_context ctr_drbg_{};
mbedtls_ssl_config conf_{};
mbedtls_x509_crt public_cert_{};
mbedtls_pk_context pk_key_{};
mbedtls_x509_crt ca_cert_{};
... which are properly init'ed in its constructor ...
mbedtls_ssl_init(&ssl_);
mbedtls_ssl_config_init(&conf_);
mbedtls_ctr_drbg_init(&ctr_drbg_);
mbedtls_entropy_init(&entropy_);
mbedtls_x509_crt_init(&public_cert_);
mbedtls_pk_init(&pk_key_);
mbedtls_x509_crt_init(&ca_cert_);
... but are never free'd ... until now
~impl()
{
mbedtls_x509_crt_free(&ca_cert_);
mbedtls_pk_free(&pk_key_);
mbedtls_x509_crt_free(&public_cert_);
mbedtls_entropy_free(&entropy_);
mbedtls_ctr_drbg_free(&ctr_drbg_);
mbedtls_ssl_config_free(&conf_);
mbedtls_ssl_free(&ssl_);
}
asio::ssl::mbedtls::engine::impl::configure calls ...
mbedtls_x509_crt_init(&public_cert_);
mbedtls_pk_init(&pk_key_);
mbedtls_x509_crt_init(&ca_cert_);
... again without first free'ing any resources that might be held by such.
now this is done first:
mbedtls_x509_crt_free(&ca_cert_);
mbedtls_pk_free(&pk_key_);
mbedtls_x509_crt_free(&public_cert_);
asio::ssl::engine has this member
std::pair<std::shared_ptr<bio>, std::shared_ptr<bio>> bio_;
which is made in its constructor
explicit engine(std::shared_ptr<context> ctx): ctx_(std::move(ctx)),
bio_(bio::new_pair("mbedtls-engine")), state_(IDLE), verify_mode_(0) {}
asio::ssl::mbedtls::bio::new_pair creates a cyclic reference between its
paired elements
static std::pair<std::shared_ptr<bio>, std::shared_ptr<bio>> new_pair(const char *error_location)
{
auto b1 = std::shared_ptr<bio>(new (std::nothrow) bio);
auto b2 = std::shared_ptr<bio>(new (std::nothrow) bio);
if (b1 == nullptr || b2 == nullptr) {
throw_alloc_failure(error_location);
} else {
b1->peer_ = b2;
b2->peer_ = b1;
}
return std::make_pair(b1, b2);
}
there is no asio::ssl::engine destructor to untie this cycle so when the
pair member is destroyed, its elements will leak.
a destructor is needed to fix this ...
~engine()
{
bio::untie_pair(bio_);
}
... along with untie_pair ...
// untie cyclic shared_ptr references made by new_pair in preparation for destruction
static void untie_pair(std::pair<std::shared_ptr<bio>, std::shared_ptr<bio>>& pair)
{
if (pair.first) {
pair.first->peer_.reset();
}
if (pair.second) {
pair.second->peer_.reset();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
asio::ssl::stream leaks a significant amount (>20KB) of memory over its lifetime.
asio::ssl::mbedtls::engine::impl has these members ...
... which are properly init'ed in its constructor ...
... but are never free'd ... until now
asio::ssl::mbedtls::engine::impl::configure calls ...
... again without first free'ing any resources that might be held by such. now this is done first:
asio::ssl::engine has this member
which is made in its constructor
asio::ssl::mbedtls::bio::new_pair creates a cyclic reference between its paired elements
there is no asio::ssl::engine destructor to untie this cycle so when the pair member is destroyed, its elements will leak.
a destructor is needed to fix this ...
... along with untie_pair ...
Related
Testing
I am writing an esphome smtp component that uses asio to send email through smtp.gmai.com.
My smtp component calls on esphome for asio support
async def to_code(config):
add_idf_component(name="espressif/asio", ref=">=1.32.0", submodules=["asio/asio"])
Without this fix, my esp cores3 would run out of memory after a few sessions.
Using esphome debug memory monitoring over the device's web interface,
it was apparent the huge memory drops were due to may smtp component's activity.
The first fix was obvious: mbedtls init'ed resources must be free'd.
After that fix, things improved considerably but there was still a leak of ~2KB/stream lifetime.
This was due to the cyclic shared_ptr references that allowed bio pair elements to be leaked.
My project is running comfortably now.
The heap memory available graph in the esphome web interface shows a steady horizontal line.
Note
Adds destructors and cleanup to mbedtls engine and unties BIO pair to eliminate memory leaks during ssl::stream lifecycle.
mbedtls_engine.hpp:engine::~engine()to callbio::untie_pair(bio_)and break cyclicshared_ptrreferences.impl::~impl()to freembedtlsresources:ssl_,conf_,ctr_drbg_,entropy_,ca_cert_,pk_key_,public_cert_.impl::configure(...), freeca_cert_,pk_key_, andpublic_cert_before re-initializing them.mbedtls_bio.hpp:bio::untie_pair(...)to resetpeer_pointers created bybio::new_pair(...).Written by Cursor Bugbot for commit 9590aec. This will update automatically on new commits. Configure here.