Skip to content

Cleanup tls client#1964

Merged
achamayou merged 11 commits into
microsoft:masterfrom
achamayou:cleanup_tls_client
Dec 1, 2020
Merged

Cleanup tls client#1964
achamayou merged 11 commits into
microsoft:masterfrom
achamayou:cleanup_tls_client

Conversation

@achamayou
Copy link
Copy Markdown
Member

fix #738

@achamayou achamayou requested a review from a team as a code owner November 30, 2020 20:57
@ghost
Copy link
Copy Markdown

ghost commented Nov 30, 2020

cleanup_tls_client@16082 aka 20201201.24 vs master ewma over 50 builds from 15423 to 16079
images

@achamayou
Copy link
Copy Markdown
Member Author

Although this doesn’t solve #1701, I think this is still a worthwhile change.

Comment thread src/clients/tls_client.h Outdated
Copy link
Copy Markdown
Member

@eddyashton eddyashton left a comment

Choose a reason for hiding this comment

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

I don't think this fixes the original linked issue. If we throw in connect(), we throw in the constructor of TlsClient, meaning we don't call the default destructor ~TlsClient(), and in turn we don't call the destructors of the members or the relevant _free functions. We either need to pepper connect() with _frees, or maybe:

TlsClient()
{
  try
  {
    connect();
  }
  catch (const std::exception&)
  {
    disconnect();
    throw;
  }
}

@achamayou
Copy link
Copy Markdown
Member Author

@eddyashton yes, you're right, I completely missed that.

Comment thread src/clients/tls_client.h Outdated
@achamayou achamayou merged commit e5b19f7 into microsoft:master Dec 1, 2020
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.

Memory leaks in crypto code in error paths

2 participants