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

Remove options memory leak during consent setting #922

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

stima
Copy link
Contributor

@stima stima commented Dec 7, 2023

Do not do early exit loop early, that leads to a incorrect reference counting of options during consent setting.

@stima
Copy link
Contributor Author

stima commented Dec 7, 2023

Not sure if it worth adding some testing around that.

@stima stima force-pushed the master branch 2 times, most recently from 3f3abc3 to de65c1a Compare December 7, 2023 17:00
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #922 (a1dbb0f) into master (0b17731) will increase coverage by 8.54%.
The diff coverage is 89.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #922      +/-   ##
==========================================
+ Coverage   74.10%   82.64%   +8.54%     
==========================================
  Files          52       53       +1     
  Lines        7170     7376     +206     
  Branches     1150     1186      +36     
==========================================
+ Hits         5313     6096     +783     
+ Misses       1310     1171     -139     
+ Partials      547      109     -438     

Copy link
Collaborator

@supervacuus supervacuus left a comment

Choose a reason for hiding this comment

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

Thanks, @stima, nice catch!

By the way, the correct approach to exit these synchronization scopes early would be to use continue instead of break. I looked through the code to see if there was another occurrence of such an early exit, but there was none. From my side, it is okay to add the nesting in this function.

Not sure if it worth adding some testing around that.

It is 💯 worth it, given that triggering this bug in a test is trivial (you only need to consent twice in a row), and our unit tests are run through ASAN in CI, which would have revealed this earlier if the branch had been covered. Not adding this case as a regression test would be careless.

Please give another consent here

sentry_user_consent_give();

and add a short comment that explains why it was done with a reference to this PR (you can also write a separate test if you want, but that should be enough). Please also add the fix to the changelog. Thx!

@supervacuus
Copy link
Collaborator

Can you please fix the formatting of your comment https://github.com/getsentry/sentry-native/actions/runs/7165776324/job/19508507795?pr=922#step:3:90 accordingly? Thx!

@supervacuus supervacuus merged commit 613f470 into getsentry:master Dec 12, 2023
19 checks passed
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.

None yet

2 participants