Fixed bug #61828 Memleak when calling Directory(Recursive)Iterator/Spl(T...

[reeze]

Hi,
This memory leak is the same as felipe ever fixed one:

commit af2fc62
Author: Felipe Pena felipe@php.net
Date: Sun Mar 11 15:42:57 2012 +0000

- Fixed memory leak when calling SplFileInfo's constructor twice

The bug affect those SPL classes:

• DirectoryIterator(".");
• RecursiveDirectoryIterator(".");
• FilesystemIterator(".");
• GlobIterator(".");
• SplFileObject('.');
• SplTempFileObject(1);

Hi, @felipensp will you take a look at this pull request?

Thank you.

[php-pulls]

Hi:
I think this is wrong.

A object is an instance, double construct it is essentially wrong.
we should fix this by prevent double constructing.

thanks

On Mon, Apr 30, 2012 at 11:42 AM, reeze
reply@reply.github.com
wrote:

Hi,
 This memory leak is the same as  felipe ever fixed one:

commit af2fc62
Author: Felipe Pena felipe@php.net
Date:   Sun Mar 11 15:42:57 2012 +0000

   - Fixed memory leak when calling SplFileInfo's constructor twice

The bug affect those SPL classes:

• DirectoryIterator(".");
• RecursiveDirectoryIterator(".");
• FilesystemIterator(".");
• GlobIterator(".");
• SplFileObject('.');
• SplTempFileObject(1);

Hi, @felipensp  will you take a look at this pull request?

Thank you.

You can merge this Pull Request by running:

 git pull https://github.com/reeze/php-src fix-stream-interator-construct-leak

Or you can view, comment on it, or merge it online at:

 #71

-- Commit Summary --

• Fixed bug #61828 Memleak when calling Directory(Recursive)Iterator/Spl(Temp)FileObject ctor twice

-- File Changes --

M ext/spl/spl_directory.c (22)
A ext/spl/tests/bug61828.phpt (26)

-- Patch Links --

 https://github.com/php/php-src/pull/71.patch
 https://github.com/php/php-src/pull/71.diff

---

Reply to this email directly or view it on GitHub:
#71

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

[reeze]

@php-pulls
but any instance can be construct any times like:

$instance->__construct();
$instance->__construct();
$instance->__construct();

so does other magic methods, should we forbid user-land magic method calling?

[php-pulls]

On Mon, Apr 30, 2012 at 12:55 PM, reeze
reply@reply.github.com
wrote:

@php-pulls
but any instance can be construct any times like:

$instance->__construct();
$instance->__construct();
$instance->__construct();

so does other magic method, should they forbidden userland call?
not all the construct creating a new instance.

thanks

---

Reply to this email directly or view it on GitHub:
#71 (comment)

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

[reeze]

It seems that constructor did create new instance when calling __constuct();

__construct(".."); var_dump($x === $y); // true

[php-pulls]

On Mon, Apr 30, 2012 at 1:29 PM, reeze
reply@reply.github.com
wrote:

It seems that constructor did create new instance when calling __constuct();

__construct(".."); var_dump($x === $y); // true you didn't understand my point, I mean, a object should be a instance of a class(like a person to people), and it should not change its features(like a sex of a person, yeah, I know there is transsexual operation in our world, but please.).

thanks

---

Reply to this email directly or view it on GitHub:
#71 (comment)

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

[smalyshev]

You should not call __construct() directly, you should use new for that.

[reeze]

It more likely state changing but not feature changing eg: $x->setProp($value)
what do you think?

"you didn't understand my point, I mean, a object should be a
instance of a class(like a person to people), and it should not
change its features(like a sex of a person, yeah, I know there is
transsexual operation in our world, but please.)."

@smalyshev yes it's wrong to do that, but users can do that way, so a simple check would be fine to prevent memleak.
or like @php-pulls (I don't know your id, sorry) said: prevent user do that is another way to prevent it.

[php-pulls]

On Mon, Apr 30, 2012 at 2:08 PM, reeze
reply@reply.github.com
wrote:

It more likely state changing but not feature changing eg: $x->setProp($value)
what do you think?
I do not think so, Constructor is named Constructor, not Setter.

"you didn't understand my point,  I mean,  a object should be a
instance of a class(like a person to people),  and it should not
change its features(like a sex of a person,  yeah, I know there is

transsexual operation in our world, but please.).

@smalyshev yes it's wrong to do that, but users can do that way, so I simple check would be fine to prevent memleak.
or like @php-pulls (I don't know your id, sorry) said: prevent user do that is another way to prevent it.
ha, sorry, it's me (laruence), if somebody reply you via mail, the
git-pull will be his agent.

what I said is your fix is not in the right way, prevent wrong usage
will be a little better,
but actually, I think it could be leaved it there, wrong use always
cause memleaks.

thanks

---

Reply to this email directly or view it on GitHub:
#71 (comment)

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

[nikic]

I agree with @reeze that this should be fixed. PHP does not prevent anyone from explicitly calling the constructor after the object was already created. Sure, you probably should not do it, but doing it should not leave you with a memleak.

[php-pulls]

On Mon, Apr 30, 2012 at 4:54 PM, nikic
reply@reply.github.com
wrote:

I agree with @reeze that this should be fixed. PHP does not prevent anyone from explicitly calling the constructor after the object was already created. Sure, you probably should not do it, but doing it should not leave you with a memleak.

sure, if we can fix it in a proper way, of course we should fix it.

but I really doubt it by allow people re-construct a object. instead,
I think do nop while doing re-construct and trigger a waring or
notice maybe better.

thanks

---

Reply to this email directly or view it on GitHub:
#71 (comment)

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

[reeze]

In user land script. he can do some work like db connection initialization in __constructor.
if he call constructor explicitly, the programmer himself is responsible for resource free.

But in SPL, SPL should responsible for it and free the resource .

[php-pulls]

On Mon, Apr 30, 2012 at 5:14 PM, reeze
reply@reply.github.com
wrote:

In user land script. he can do some work like db connection initialization in __constructor.
if he call constructor explicitly, the programmer him is responsible for resource free.

But in SPL, SPL should responsible for it and free the resource .
I know your have work on this, and I really understand you want your
work to be merged.

but please, except the fixing way, let's focus on your fix, as I
told you today before, when we trying to fix something, we should be
very careful, and make sure we really understood what the depends
codes means.

what I said is, plz don't fix something by rush.

like your fix, I have to say it's wrong, did you test with it? like:

__construct(1); } catch (Exception $e) { } try { $a->__construct(1); } catch (Exception $e) { } will cause double free.. thanks > --- > > Reply to this email directly or view it on GitHub: > https://github.com//pull/71#issuecomment-5414042 > > ## > > Git Pull Requests Mailing List (https://github.com/php) > To unsubscribe, visit: http://www.php.net/unsub.php ## Laruence  Xinchen Hui http://www.laruence.com/

[reeze]

oh, thanks for point out this edge case
thanks , I will update the patch.

reeze | simpie.com

在 2012年4月30日星期一，下午5:51，Account for PHP Pull Requests 写道：

On Mon, Apr 30, 2012 at 5:14 PM, reeze
<reply@reply.github.com (mailto:reply@reply.github.com)>
wrote:

In user land script. he can do some work like db connection initialization in __constructor.
if he call constructor explicitly, the programmer him is responsible for resource free.

But in SPL, SPL should responsible for it and free the resource .
I know your have work on this, and I really understand you want your
work to be merged.

but please, except the fixing way, let's focus on your fix, as I
told you today before, when we trying to fix something, we should be
very careful, and make sure we really understood what the depends
codes means.

what I said is, plz don't fix something by rush.

like your fix, I have to say it's wrong, did you test with it? like:

__construct(1); } catch (Exception $e) { } try { $a->__construct(1); } catch (Exception $e) { } will cause double free.. thanks > --- > > Reply to this email directly or view it on GitHub: > https://github.com//pull/71#issuecomment-5414042 > > ## > > Git Pull Requests Mailing List (https://github.com/php) > To unsubscribe, visit: http://www.php.net/unsub.php ## Laruence Xinchen Hui http://www.laruence.com/ --- Reply to this email directly or view it on GitHub: https://github.com//pull/71#issuecomment-5414488

[php-pulls]

On Mon, Apr 30, 2012 at 6:13 PM, reeze
reply@reply.github.com
wrote:

oh, thanks for point out this edge case
It's not a edge case,

thanks

thanks , I will update the patch.

reeze | simpie.com

在 2012年4月30日星期一，下午5:51，Account for PHP Pull Requests 写道：

On Mon, Apr 30, 2012 at 5:14 PM, reeze
<reply@reply.github.com (mailto:reply@reply.github.com)>
wrote:

In user land script. he can do some work like db connection initialization in __constructor.
if he call constructor explicitly, the programmer him is responsible for resource free.

But in SPL, SPL should responsible for it and free the resource .
I know your have work on this, and I really understand you want your
work to be merged.

but please, except the fixing way, let's focus on your fix, as I
told you today before, when we trying to fix something, we should be
very careful, and make sure we really understood what the depends
codes means.

what I said is, plz don't fix something by rush.

like your fix, I have to say it's wrong, did you test with it? like:

__construct(1); } catch (Exception $e) { } try {  $a->__construct(1); } catch (Exception $e) { } will cause double free.. thanks > --- > > Reply to this email directly or view it on GitHub: > https://github.com//pull/71#issuecomment-5414042 > > ## > > Git Pull Requests Mailing List (https://github.com/php) > To unsubscribe, visit: http://www.php.net/unsub.php ## Laruence Xinchen Hui http://www.laruence.com/ --- Reply to this email directly or view it on GitHub: https://github.com//pull/71#issuecomment-5414488

---

Reply to this email directly or view it on GitHub:
#71 (comment)

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

[reeze]

you are right ，more work needed to get this fixed

Reeze
Sent

On 2012年4月30日Monday at 18:37, Account for PHP Pull Requests wrote:

The content of the message has not been downloaded yet.

[felipensp]

I've tested all PHP classes with direct constructor calls (using my fuzzer) and just got those memleaks from some SPL classes at the moment. So I guess it's ok to fix that case, just like I did for other class.

[colder]

Hi,

this patch makes sense.

Internal classes should be as robust as possible, they should not rely on "best-usage" principles from userland code.

Best,

[reeze]

thanks. this patch need to be improved to handle certain cases provided by laruence, I will update it later.

[reeze]

Hi, guys:
I've udpated the patch. add test case udpated too.
the commit fixed: string convertion will segfault when the second construct failed by thrown exception(catched) and other memleaks.

Thanks.