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

refactor: Make Tox_Options own the passed proxy host and savedata. #2819

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Dec 28, 2024

This way, client code can immediately free their data after the options setter returns true.


This change is Reviewable

@iphydf iphydf added this to the v0.2.21 milestone Dec 28, 2024
@github-actions github-actions bot added the refactor Refactoring production code, eg. renaming a variable, not affecting semantics label Dec 28, 2024
@iphydf iphydf force-pushed the owned-options branch 2 times, most recently from 2e5d814 to fdd1be5 Compare December 28, 2024 19:13
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

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

can immediately free their data and can pass temporaries to the options setters

This contradicts with:

* The setter tries to copy the string, but if it fails, the value is not
* copied and this member is set to the user-provided pointer. In that
* case, the user must not free the string until the Tox_Options object is
* freed. Client code can check whether allocation succeeded by comparing
* the value of this member to the user-provided pointer.
*/

Looks like the client has to check first if the copying succeeded before they free the data, so not exactly immediately, but could be close enough based on what you meant with that comment. As compared to the previous behavior, you can free it sooner, yes.

Also, if the user clones Tox_Options, freeing it will free the pointers twice.

@iphydf iphydf force-pushed the owned-options branch 4 times, most recently from 523340a to ccef3ea Compare December 29, 2024 01:22
toxcore/tox.h Show resolved Hide resolved
toxcore/tox.h Outdated Show resolved Hide resolved
@iphydf iphydf force-pushed the owned-options branch 4 times, most recently from d32d4a3 to 987ad78 Compare January 11, 2025 01:26
@iphydf iphydf requested a review from Green-Sky January 12, 2025 17:34
@iphydf iphydf force-pushed the owned-options branch 2 times, most recently from 13df70a to ce30952 Compare January 13, 2025 13:25
toxcore/tox_api.c Show resolved Hide resolved
tox_options_set_savedata_type(options, TOX_SAVEDATA_TYPE_TOX_SAVE);

tox_options_set_savedata_data(options, clear, size);
tox_options_set_savedata_data(options, clear, clear_size);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be ck_asserted so there's no potential for a use-after-free in the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good idea. Done.

toxcore/tox_api.c Show resolved Hide resolved
@iphydf iphydf force-pushed the owned-options branch 4 times, most recently from 386be37 to ebe150f Compare January 16, 2025 09:26
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 73.46939% with 13 lines in your changes missing coverage. Please review.

Project coverage is 72.18%. Comparing base (aa64916) to head (ebe150f).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
toxcore/tox_api.c 73.46% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2819      +/-   ##
==========================================
- Coverage   72.21%   72.18%   -0.04%     
==========================================
  Files         151      151              
  Lines       31122    31176      +54     
==========================================
+ Hits        22476    22505      +29     
- Misses       8646     8671      +25     

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

This way, client code can immediately free their data and can pass
temporaries to the options setters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants