Skip to content

Conversation

krakjoe
Copy link
Member

@krakjoe krakjoe commented Sep 7, 2025

No description provided.

return NULL;
}
phar->fname = source->fname;
phar->fname = estrndup(source->fname, source->fname_len);
Copy link
Member Author

@krakjoe krakjoe Sep 7, 2025

Choose a reason for hiding this comment

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

maybe expect pestrndup, it is not necessary because the new phar object is calloced (so is_persistent is set 0), so that when phar_destroy_phar_data executes the string will be efree'd correctly.

@nielsdos
Copy link
Member

nielsdos commented Sep 7, 2025

I predict this will leak memory for sure. I think my approach that I posted in the thread is the right one. But as I said, I still have to verify this. Why try to race against me to make a PR though?

@krakjoe
Copy link
Member Author

krakjoe commented Sep 7, 2025

Why try to race against me to make a PR though?

There's no race, you are doing other things, said you "didn't have time", and the solution didn't appear correct to me.

@nielsdos
Copy link
Member

nielsdos commented Sep 7, 2025

I said I don't have time to double check now ;) Which means I will check it in the near future :P
Your solution isn't right because at one point the rename code will swap out the filename, so it will leak. The rename code can error out prior to the reassignment of the filename, which is why the test causes a crash. The rename code can also error out at a later point, which means it will have already assigned the new filename. In my solution: we detect in which case we are in and act accordingly.

@krakjoe
Copy link
Member Author

krakjoe commented Sep 7, 2025

Indeed rename does re-assign it, it will leak ...

@krakjoe krakjoe closed this Sep 7, 2025
@krakjoe krakjoe deleted the krakjoe/gh-19752 branch September 7, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants