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

free strdupped temporary variable in joinpath #1438

Merged
merged 1 commit into from
Apr 4, 2018
Merged

free strdupped temporary variable in joinpath #1438

merged 1 commit into from
Apr 4, 2018

Conversation

DrDaveD
Copy link
Collaborator

@DrDaveD DrDaveD commented Apr 3, 2018

Description of the Pull Request (PR):

I noticed a strdup() in joinpath() that was not getting freed and not needed after the function returns. This adds the free().

This fixes or addresses the following GitHub issues:

  • None

Checkoff for all PRs:

  • I have read the Guidelines for Contributing, and this PR conforms to the stated requirements.
  • I have added changes to the CHANGELOG and and documentation updates to the singularityware documentation base.
  • I have tested this PR locally with a make test
  • This PR is NOT against the project's master branch
  • I have added myself as a contributor to the contributors's file
  • This PR is ready for review and/or merge

Attn: @singularityware-admin

Copy link
Collaborator

@GodloveD GodloveD left a comment

Choose a reason for hiding this comment

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

Nice catch. Should the free also go into the preceding if statement just in case the function bombs out?

@@ -191,6 +191,8 @@ char *joinpath(const char * path1, const char * path2_in) {
ABORT(255);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the free also go in this if statement above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't think it was worth doing since the program dies there anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your right.

@tri-adam tri-adam merged commit 21ec06b into apptainer:development-2.x Apr 4, 2018
@lyklev
Copy link

lyklev commented Apr 19, 2018

I noticed that indeed where joinpath and strdup are used, most of the time it leaks memory. It was my intention to fix it in a few places, but the memory leaking is quite common. I have started working on it, and will probably fix the memory leaks in small steps, in many commits to make the fix easier to test and review.

@lyklev
Copy link

lyklev commented Apr 19, 2018

Yes, it is, it is one of the many places where strdup without a free leaks memory.

It was my intention to fix the memory leaks bit by bit, mostly by using static variables where possible. In fact, some of the fixes are already in my "strings-fix" branch for which I have not made any pull requests yet - but feel free to have a look.

@DrDaveD DrDaveD deleted the joinpath-strdup-free branch July 11, 2018 09:09
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.

5 participants