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: Free memory returned by backtrace_symbols() in debug builds #3202

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

indragiek
Copy link
Member

📜 Description

This code is only compiled in debug builds, but we were forgetting to free the pointer returned by backtrace_symbols. The documentation (https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/backtrace_symbols.3.html) says:

It is allocated using malloc() and should be released using free()

💡 Motivation and Context

Customer reported a memory leak in a call to backtrace_symbols, this is the cause.

💚 How did you test it?

Verify that the memory is freed and does not leak in a debug build.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

This code is only compiled in debug builds, but we were forgetting to `free` the pointer returned by `backtrace_symbols`. The documentation (https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/backtrace_symbols.3.html)  says:

> It is allocated using malloc() and should be released using free()
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #3202 (2f5905e) into main (25e65a5) will decrease coverage by 0.051%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3202       +/-   ##
=============================================
- Coverage   89.190%   89.139%   -0.051%     
=============================================
  Files          502       502               
  Lines        54034     54022       -12     
  Branches     19386     19376       -10     
=============================================
- Hits         48193     48155       -38     
- Misses        4988      5008       +20     
- Partials       853       859        +6     
Files Changed Coverage Δ
Sources/Sentry/Profiling/SentryProfilerState.mm 98.425% <100.000%> (+0.038%) ⬆️

... and 13 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25e65a5...2f5905e. Read the comment docs.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Checks out, only usage of that is under DEBUG builds.

@github-actions
Copy link

github-actions bot commented Aug 3, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.61 ms 1253.64 ms 18.03 ms
Size 22.84 KiB 403.19 KiB 380.34 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
cbf6225 1221.73 ms 1251.20 ms 29.47 ms
bd2afa6 1245.24 ms 1263.18 ms 17.94 ms
11ccc16 1206.45 ms 1225.60 ms 19.15 ms
28333b6 1247.29 ms 1262.51 ms 15.22 ms
ff5c1d8 1244.30 ms 1263.96 ms 19.66 ms
4654f66 1247.16 ms 1263.96 ms 16.80 ms
3f366ee 1242.28 ms 1260.80 ms 18.52 ms
67460f4 1244.56 ms 1255.96 ms 11.40 ms
8f397a7 1236.76 ms 1256.76 ms 20.00 ms
1bf8571 1215.31 ms 1232.48 ms 17.17 ms

App size

Revision Plain With Sentry Diff
cbf6225 20.76 KiB 425.77 KiB 405.00 KiB
bd2afa6 20.76 KiB 420.55 KiB 399.79 KiB
11ccc16 20.76 KiB 431.71 KiB 410.94 KiB
28333b6 20.76 KiB 424.69 KiB 403.93 KiB
ff5c1d8 20.76 KiB 430.98 KiB 410.22 KiB
4654f66 20.76 KiB 432.17 KiB 411.41 KiB
3f366ee 20.76 KiB 427.84 KiB 407.08 KiB
67460f4 20.76 KiB 426.15 KiB 405.39 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
1bf8571 20.76 KiB 437.12 KiB 416.36 KiB

Previous results on branch: indragiek/free-backtrace-symbols

Startup times

Revision Plain With Sentry Diff
78fc65a 1232.78 ms 1242.96 ms 10.18 ms

App size

Revision Plain With Sentry Diff
78fc65a 22.84 KiB 403.19 KiB 380.34 KiB

@indragiek indragiek merged commit ca91a5c into main Aug 3, 2023
64 of 66 checks passed
@indragiek indragiek deleted the indragiek/free-backtrace-symbols branch August 3, 2023 21:27
vaind pushed a commit that referenced this pull request Aug 10, 2023
* fix: Free memory returned by backtrace_symbols() in debug builds

This code is only compiled in debug builds, but we were forgetting to `free` the pointer returned by `backtrace_symbols`. The documentation (https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/backtrace_symbols.3.html)  says:

> It is allocated using malloc() and should be released using free()

* Update CHANGELOG.md
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