Skip to content

Fixed bug #61964 (finfo_open with directory cause invalid free) #91

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.

Sign up for GitHub

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

Jump to bottom
Merged
php-pulls merged 1 commit into from
Jul 15, 2012
Merged

Fixed bug #61964 (finfo_open with directory cause invalid free) #91

php-pulls merged 1 commit into from
Jul 15, 2012

Conversation

@reeze
Copy link
Contributor

@reeze reeze commented May 24, 2012

Hi,
we have been discuss this issue in irc, @laruence got some concern,
then I sent a mail to the file lib author: Christos Zoulas christos@zoulas.com.
he replied:

I got two question to ask:

  1. is file intend to support dir?
  2. does the double free is bug?
    Yes, yes, and I don't know what version you are using but the current version
    does asprintf() not snprintf().

both 5.3&5.4 didn't use asprintf(), we use our memory management functions.

after got his reply I think I can send this PR to ask you guys comment.

Thanks.

@laruence
Copy link
Member

laruence commented May 25, 2012

..... Double free is no doubt a bug, the reason I care is lib magic is not ready for that in php, as I said there even was code fprintf(stderr... So I think it should be whole examed by somebody who really know it. But not do a little fix one by one,

Thanks

@reeze
Copy link
Contributor Author

reeze commented May 25, 2012

Hm,.. from the commit log , seems Felipe & Pierre knows about it, we could ask them to improve it.

@nikic
Copy link
Member

nikic commented May 25, 2012

Thanks for looking into this @reeze :)

@laruence
Copy link
Member

laruence commented May 25, 2012

for the record, as felipe said, this patch cause segfault. need to be re-examed.
it's really bad we having two conversiones both at here and bugs.php.net...

@php-pulls
Copy link

php-pulls commented Jul 7, 2012

Comment on behalf of stas at php.net:

Looks like the patch is not right (see the bug, it segfaults) so please fix and resubmit the pull, I'll close this one for now.

@reeze
Copy link
Contributor Author

reeze commented Jul 8, 2012

Hi, stas:
The segfault mentioned by felipe have been fixed right after felipe reported in the bug, and I've added the test case:

https://github.com/php/php-src/pull/91/files#L1R31.

Thanks for reply!

@reeze
Copy link
Contributor Author

reeze commented Jul 8, 2012

BTW: The reason there is only on commit is that I want to make the commit clean I update the PR by force-update.

@php-pulls php-pulls merged commit 1d2f619 into php:master Jul 15, 2012
@dunglas dunglas mentioned this pull request Sep 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@reeze @laruence @nikic @php-pulls