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

Fix memory leaks in Windows TLS code #652

Merged
merged 5 commits into from
Jul 10, 2024
Merged

Fix memory leaks in Windows TLS code #652

merged 5 commits into from
Jul 10, 2024

Conversation

graebm
Copy link
Contributor

@graebm graebm commented Jul 3, 2024

Issue:

A C++ SDK user found a memory leak in the Windows TLS channel-handler code and submitted the following PR: #651

In evaluating this 2-line fix, I noticed that it sat in the middle of some (preexisting) sloppy error-handling.

Description of changes:

  • Fix memory leak, as shown in @normanade's PR, by calling FreeContextBuffer().
    • Change it so we always call FreeContextBuffer() after calling to AcceptSecurityContext()/InitializeSecurityContextA(). Previously we'd only free the buffer in certain circumstances, and were probably leaking.
  • Remove (often sloppy) error-handling that tries to deal with allocation failures of aws_io_message. Officially declare that message allocations can't fail.
    • NOTE: Ever since this change in 2021, our allocators will never return NULL. They deal with OOM by immediately terminating the program. This was done because error-handling code for allocation failure makes everything so much more complex, and it's seldom tested. We tried for years to handle it. It's not worth the effort.
  • Fix some error-handling that forgot to delete the aws_io_message after checking its capacity
  • In the Windows TLS code, replace some AWS_ASSERT() about buffers being large enough with AWS_FATAL_ASSERT(). Similarly, add an AWS_FATAL_ASSERT() in one place that never checked or asserted before doing the copy.
    • I don't know enough about TLS or Windows APIs to say whether or not these deserve real error-handling. So I'm just preserving the status quo (but crashing instead of doing undefined behavior).

Co-authored-by: @normanade

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@graebm graebm changed the title Fix win memleaks Fix memory leaks in Windows TLS code Jul 3, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.22%. Comparing base (9072f86) to head (5720360).

Files Patch % Lines
source/channel.c 66.66% 1 Missing ⚠️
source/s2n/s2n_tls_channel_handler.c 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #652      +/-   ##
==========================================
+ Coverage   80.12%   80.22%   +0.10%     
==========================================
  Files          28       28              
  Lines        5967     5957      -10     
==========================================
- Hits         4781     4779       -2     
+ Misses       1186     1178       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@graebm graebm merged commit 0e65ff4 into main Jul 10, 2024
36 checks passed
@graebm graebm deleted the fix-win-memleaks branch July 10, 2024 16:58
graebm added a commit to awslabs/aws-crt-cpp that referenced this pull request Jul 11, 2024
Update to latest submodules:
```
aws-c-cal          v0.7.0 -> v0.7.1
aws-c-io           v0.14.9 -> v0.14.11
aws-c-http         v0.8.2 -> v0.8.3
aws-c-s3           v0.5.10 -> v0.6.1
aws-lc             v1.30.1 -> v1.31.0
s2n                v1.4.16 -> v1.4.17
```

Fix comes from here: awslabs/aws-c-io#652
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.

4 participants