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.

Sign up for GitHub

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

Jump to bottom

Fix memory leak in ssl_server2 when buffer allocator and backtracing enabled #2070

Merged
merged 2 commits into from
Oct 29, 2018
Merged

Fix memory leak in ssl_server2 when buffer allocator and backtracing enabled #2070

merged 2 commits into from
Oct 29, 2018

Conversation

hanno-becker
Copy link

Fixes #2069.

Hanno Becker added 2 commits October 9, 2018 12:44
Fix ordering of free()ing of internal structures in ssl_server2
095d9cf 
If `MBEDTLS_MEMORY_BUFFER_ALLOC_C` is configured and Mbed TLS'
custom buffer allocator is used for calloc() and free(), the
read buffer used by the server example application is allocated
from the buffer allocator, but freed after the buffer allocator
has been destroyed. If memory backtracing is enabled, this leaves
a memory leak in the backtracing structure allocated for the buffer,
as found by valgrind.

Fixes Mbed-TLS#2069.
Adapt ChangeLog
abe6003
@hanno-becker hanno-becker added bug needs-review Every commit must be reviewed by at least two team members, component-tls labels Oct 9, 2018
This was referenced Oct 9, 2018
Merged
Merged
Closed
RonEld
RonEld approved these changes Oct 10, 2018
View reviewed changes
Copy link
Contributor

@RonEld RonEld left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving, as this is the correct sequence of operations. However, memory is not being leaked, as mbedtls_memory_buffer_alloc_free() clears the whole heap, and the next call to mbedtls_free() actually doesn't do anything. Nonetheless, this should be fixed

@hanno-becker
Copy link
Author

hanno-becker commented Oct 10, 2018 •
edited
Loading

@RonEld No, as mentioned in the commit message the problem is that internally the buffer allocator uses the backtrace_symbols() call which does call the system malloc() under the hood. So there is an actual memory leak. See backtrace_symbols(), especially:

This array is malloc(3)ed by backtrace_symbols(), and must be freed by the caller. (The strings pointed to by the array of pointers need not and should not be freed.)

@RonEld
Copy link
Contributor

RonEld commented Oct 10, 2018

ok

@simonbutcher simonbutcher removed their request for review October 10, 2018 14:26
@simonbutcher simonbutcher added needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 10, 2018
@simonbutcher
Copy link
Contributor

retest

@simonbutcher simonbutcher added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Oct 12, 2018
@simonbutcher simonbutcher merged commit abe6003 into Mbed-TLS:development Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RonEld RonEld RonEld approved these changes

@AndrzejKurek AndrzejKurek AndrzejKurek approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-tls
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hanno-becker @RonEld @simonbutcher @AndrzejKurek