Fix memory leak on Randomizer::__construct() call twice #9091
Conversation
ext/random/randomizer.c
Outdated
| @@ -80,7 +85,7 @@ PHP_METHOD(Random_Randomizer, __construct) | |||
|
|
|||
| ZVAL_OBJ(&zengine_object, engine_object); | |||
|
|
|||
| zend_update_property(random_ce_Random_Randomizer, Z_OBJ_P(ZEND_THIS), "engine", strlen("engine"), &zengine_object); | |||
| zend_update_property(random_ce_Random_Randomizer, Z_OBJ_P(ZEND_THIS), "engine", sizeof("engine") - 1, &zengine_object); | |||
There was a problem hiding this comment.
Not sure how this change is related, but that's still strlen("engine") in the zend_read_property call near the end of this file
php-src/ext/random/randomizer.c
Line 282 in e9a447f
There was a problem hiding this comment.
Thank you. This is a digression from the purpose of this PR, but it was minor and has been corrected.
4afc9db
There was a problem hiding this comment.
Please don't bundle unrelated changes into a PR, it distracts from the relevant parts. Can you please revert the strlen / sizeof changes?
I also disagree with using sizeof in principle: Modern compilers can optimize both. For older compilers the string is very short and it's also not in a hot path. strlen() is much more readable, because the - 1 is not needed.
There was a problem hiding this comment.
That's right.
But, the current situation where it is not unified is not so good, so we will create a separate PR to unify it with strlen.
ext/random/randomizer.c
Outdated
| @@ -80,7 +85,7 @@ PHP_METHOD(Random_Randomizer, __construct) | |||
|
|
|||
| ZVAL_OBJ(&zengine_object, engine_object); | |||
|
|
|||
| zend_update_property(random_ce_Random_Randomizer, Z_OBJ_P(ZEND_THIS), "engine", strlen("engine"), &zengine_object); | |||
| zend_update_property(random_ce_Random_Randomizer, Z_OBJ_P(ZEND_THIS), "engine", sizeof("engine") - 1, &zengine_object); | |||
There was a problem hiding this comment.
Please don't bundle unrelated changes into a PR, it distracts from the relevant parts. Can you please revert the strlen / sizeof changes?
I also disagree with using sizeof in principle: Modern compilers can optimize both. For older compilers the string is very short and it's also not in a hot path. strlen() is much more readable, because the - 1 is not needed.
Co-authored-by: Tim Düsterhus <timwolla@googlemail.com>
This reverts commit 4afc9db.
Either way, updating the readonly property will result in an error, so an exception is thrown in accordance with the Phar implementation.
Closes GH-9089