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

ICU-22825 Fix memLeak during error in tznames_impl.cpp #3081

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

FrankYFTang
Copy link
Contributor

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22825
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@FrankYFTang FrankYFTang requested review from sffc and richgillam August 2, 2024 09:37
richgillam
richgillam previously approved these changes Aug 2, 2024
icu4c/source/i18n/tznames_impl.cpp Outdated Show resolved Hide resolved
Rewrite the TextTrieMap::put() which should delete the value
during error instead of deleting key.
Rewrite to simplified the error handling.
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/tznames_impl.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

I think this is right. I was concerned that you might need to take ownership of both the key and the value, but the documentation says this particular function is for cases where the key is in static memory owned by somebody else (the resource bundle), so I think you're doing the right thing.

@FrankYFTang
Copy link
Contributor Author

@sffc could you double check since you are the one who wrote the origional code.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

The minimal fix would be to change fValueDeleter((void*) key); to fValueDeleter(value);

The rest of the refactor looks fine.

@FrankYFTang
Copy link
Contributor Author

The minimal fix would be to change fValueDeleter((void*) key); to fValueDeleter(value);

no, that will not be enugh because for the last line of
fLazyContents->addElement(value, status); in the origional line 255
we also need a "if fail if deleter delete block" in case the return status faile also. That is the leak the reporter detected.

@FrankYFTang FrankYFTang dismissed markusicu’s stale review August 6, 2024 16:58

addressed in other file

@FrankYFTang FrankYFTang merged commit a22dc93 into unicode-org:main Aug 6, 2024
99 checks passed
@FrankYFTang FrankYFTang deleted the ICU-22825-tznames_impl branch August 6, 2024 16:58
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