Skip to content

Fix memory leak on Randomizer::__construct() call twice #9091

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

Merged
merged 12 commits into from
Jul 23, 2022
Merged

Fix memory leak on Randomizer::__construct() call twice #9091

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e9a447f
random: fix __construct() call twice
zeriyoshi Jul 22, 2022
4adebfd
update NEWS
zeriyoshi Jul 22, 2022
a095e45
[CI skip] fix typo
zeriyoshi Jul 22, 2022
fc643aa
Merge branch 'master' into randomizer_constructor_twice
zeriyoshi Jul 22, 2022
4afc9db
fix: replace all strlen -> sizeof - 1
zeriyoshi Jul 22, 2022
e9b9118
Update ext/random/tests/03_randomizer/construct_twice.phpt
zeriyoshi Jul 23, 2022
8d38061
Revert "fix: replace all strlen -> sizeof - 1"
zeriyoshi Jul 23, 2022
31b7f9a
random: revert strlen -> sizeof - 1
zeriyoshi Jul 23, 2022
cb5f7ae
random: test: fix test readability
zeriyoshi Jul 23, 2022
e26f709
Merge remote-tracking branch 'upstream/master' into randomizer_constr…
zeriyoshi Jul 23, 2022
da07c17
random: revert sizeof - 1 in randomizer.c
zeriyoshi Jul 23, 2022
8708360
Update NEWS
zeriyoshi Jul 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions ext/random/randomizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ static inline void randomizer_common_init(php_random_randomizer *randomizer, zen
zend_string *mname;
zend_function *generate_method;

mname = zend_string_init("generate", strlen("generate"), 0);
mname = zend_string_init("generate", sizeof("generate") - 1, 0);
generate_method = zend_hash_find_ptr(&engine_object->ce->function_table, mname);
zend_string_release(mname);

Expand Down Expand Up @@ -70,6 +70,11 @@ PHP_METHOD(Random_Randomizer, __construct)
Z_PARAM_OBJ_OF_CLASS_OR_NULL(engine_object, random_ce_Random_Engine);
ZEND_PARSE_PARAMETERS_END();

if (randomizer->algo) {
zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "Cannot call constructor twice");
RETURN_THROWS();
}

/* Create default RNG instance */
if (!engine_object) {
engine_object = random_ce_Random_Engine_Secure->create_object(random_ce_Random_Engine_Secure);
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

zengine = zend_read_property(randomizer->std.ce, &randomizer->std, "engine", strlen("engine"), 0, NULL);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. This is a digression from the purpose of this PR, but it was minor and has been corrected.
4afc9db

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


randomizer_common_init(randomizer, engine_object);
}
Expand Down
36 changes: 36 additions & 0 deletions ext/random/tests/03_randomizer/construct_twice.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
--TEST--
Random: Randomizer: __construct() twice
--FILE--
<?php
try {
(new \Random\Randomizer())->__construct();
} catch (\BadMethodCallException $e) {
echo $e->getMessage() . PHP_EOL;
}

try {
(new \Random\Randomizer(
new class () implements \Random\Engine {
public function generate(): string
{
return \random_bytes(4); /* 32-bit */
}
}
))->__construct(
new class () implements \Random\Engine {
public function generate(): string
{
return \random_bytes(4); /* 32-bit */
}
}
);
} catch (\BadMethodCallException $e) {
echo $e->getMessage() . PHP_EOL;
}

die('success');
?>
--EXPECT--
Cannot call constructor twice
Cannot call constructor twice
success