8000 Fix bug #66091 (memory leak in DateTime::createFromFormat()) by Entea · Pull Request #766 · php/php-src · GitHub
[go: up one dir, main page]
Skip to content

Fix bug #66091 (memory leak in DateTime::createFromFormat())#766

Closed
Entea wants to merge 1 commit intophp:masterfrom
Entea:master
Closed

Fix bug #66091 (memory leak in DateTime::createFromFormat())#766
Entea wants to merge 1 commit intophp:masterfrom
Entea:master

Conversation

@Entea
Copy link
@Entea Entea commented Aug 9, 2014

No description provided.

@derickr
Copy link
Member
derickr commented Aug 11, 2014

This looks wrong. It will free the return_value altogether, and not just the data associated with it. Shouldn't it be zval_ptr_dtor() instead?

@Entea
Copy link
Author
Entea commented Aug 11, 2014

@derickr Thanks for the review! Does it look good to merge now? :)

@datibbaw
Copy link
Contributor
datibbaw commented Aug 12, 2014

I've tried your patch on master with the following code:

for ( $i = 0; $i < 10000; $i++ ) {
    $d = DateTime::createFromFormat('m-d-Y', 'asdf asdf');
    unset($d);

    if ($i % 100 == 0) {
        echo 'Memory usage: ', memory_get_usage(), PHP_EOL;
    }
}

The memory usage keeps going down, becomes negative and then crashes.

@Entea
Copy link
Author
Entea commented Aug 12, 2014

@datibbaw thanks for feedback! I've run this test code against patched master and didn't see any memory usage going down. Could you post you ./configure parameters?

@datibbaw
Copy link
Contributor
datibbaw commented Aug 12, 2014

@Entea I've managed to reproduce it on two systems now. This is my config.nice:

'./configure' \
'--disable-all' \
'--enable-maintainer-zts' \
'--with-openssl=/usr/local/opt/openssl' \
'--with-libedit' \
'YACC=/usr/local/opt/bison27/bin/bison' \
'--enable-json' \
"$@"

@datibbaw datibbaw mentioned this pull request Aug 13, 2014
Closed
@smalyshev smalyshev added the Bug label Nov 24, 2014
@nikic
Copy link
< 9111 div class="d-none d-sm-flex"> Member
nikic commented Mar 3, 2016

This PR has been superseded by #768, which has been merged.

@nikic nikic closed this Mar 3, 2016
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

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Entea @derickr @datibbaw @nikic @smalyshev
0