Skip to content

Fix Enum::from/tryFrom memory leak in JIT #8633

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.

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 9 additions & 1 deletion Zend/tests/enum/internal_enums.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ var_dump(ZendTestStringEnum::Foo->value);
var_dump($bar = ZendTestStringEnum::from("Test2"));
var_dump($bar === ZendTestStringEnum::Bar);
var_dump(ZendTestStringEnum::tryFrom("Test3"));
var_dump(ZendTestStringEnum::tryFrom(42));
var_dump(ZendTestStringEnum::tryFrom(43));
var_dump(ZendTestStringEnum::tryFrom(0));
var_dump(ZendTestStringEnum::cases());

var_dump($s = serialize($foo));
Expand All @@ -47,13 +50,18 @@ string(5) "Test1"
enum(ZendTestStringEnum::Bar)
bool(true)
NULL
array(3) {
enum(ZendTestStringEnum::FortyTwo)
NULL
NULL
array(4) {
[0]=>
enum(ZendTestStringEnum::Foo)
[1]=>
enum(ZendTestStringEnum::Bar)
[2]=>
enum(ZendTestStringEnum::Baz)
[3]=>
enum(ZendTestStringEnum::FortyTwo)
}
string(30) "E:22:"ZendTestStringEnum:Foo";"
enum(ZendTestStringEnum::Foo)
Expand Down
28 changes: 28 additions & 0 deletions Zend/tests/enum/internal_enums_strict_types.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
--TEST--
Internal enums from/tryFrom in strict_types=1
--EXTENSIONS--
zend_test
--FILE--
<?php

declare(strict_types=1);

var_dump(ZendTestStringEnum::from("Test2"));

try {
var_dump(ZendTestStringEnum::from(42));
} catch (Error $e) {
echo $e->getMessage(), "\n";
}

try {
var_dump(ZendTestStringEnum::tryFrom(43));
} catch (Error $e) {
echo $e->getMessage(), "\n";
}

?>
--EXPECT--
enum(ZendTestStringEnum::Bar)
ZendTestStringEnum::from(): Argument #1 ($value) must be of type string, int given
ZendTestStringEnum::tryFrom(): Argument #1 ($value) must be of type string, int given
48 changes: 40 additions & 8 deletions Zend/zend_enum.c
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ static ZEND_NAMED_FUNCTION(zend_enum_cases_func)
static void zend_enum_from_base(INTERNAL_FUNCTION_PARAMETERS, bool try)
{
zend_class_entry *ce = execute_data->func->common.scope;
bool release_string = false;
zend_string *string_key;
zend_long long_key;

Expand All @@ -221,17 +222,33 @@ static void zend_enum_from_base(INTERNAL_FUNCTION_PARAMETERS, bool try)

case_name_zv = zend_hash_index_find(ce->backed_enum_table, long_key);
} else {
ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR(string_key)
ZEND_PARSE_PARAMETERS_END();

ZEND_ASSERT(ce->enum_backing_type == IS_STRING);

if (ZEND_ARG_USES_STRICT_TYPES()) {
ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR(string_key)
ZEND_PARSE_PARAMETERS_END();
} else {
// We allow long keys so that coercion to string doesn't happen implicitly. The JIT
// skips deallocation of params that don't require it. In the case of from/tryFrom
// passing int to from(int|string) looks like no coercion will happen, so the JIT
// won't emit a dtor call. Thus we allocate/free the string manually.
ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR_OR_LONG(string_key, long_key)
ZEND_PARSE_PARAMETERS_END();

if (string_key == NULL) {
release_string = true;
string_key = zend_long_to_str(long_key);
}
}

case_name_zv = zend_hash_find(ce->backed_enum_table, string_key);
}

if (case_name_zv == NULL) {
if (try) {
RETURN_NULL();
goto return_null;
}

if (ce->enum_backing_type == IS_LONG) {
Expand All @@ -240,7 +257,7 @@ static void zend_enum_from_base(INTERNAL_FUNCTION_PARAMETERS, bool try)
ZEND_ASSERT(ce->enum_backing_type == IS_STRING);
zend_value_error("\"%s\" is not a valid backing value for enum \"%s\"", ZSTR_VAL(string_key), ZSTR_VAL(ce->name));
}
RETURN_THROWS();
goto throw;
}

// TODO: We might want to store pointers to constants in backed_enum_table instead of names,
Expand All @@ -251,11 +268,26 @@ static void zend_enum_from_base(INTERNAL_FUNCTION_PARAMETERS, bool try)
zval *case_zv = &c->value;
if (Z_TYPE_P(case_zv) == IS_CONSTANT_AST) {
if (zval_update_constant_ex(case_zv, c->ce) == FAILURE) {
RETURN_THROWS();
goto throw;
}
}

ZVAL_COPY(return_value, case_zv);
if (release_string) {
zend_string_release(string_key);
}
RETURN_COPY(case_zv);

throw:
if (release_string) {
zend_string_release(string_key);
}
RETURN_THROWS();

return_null:
if (release_string) {
zend_string_release(string_key);
}
RETURN_NULL();
}

static ZEND_NAMED_FUNCTION(zend_enum_from_func)
Expand Down
1 change: 1 addition & 0 deletions ext/zend_test/test.stub.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ enum ZendTestStringEnum: string {
case Foo = "Test1";
case Bar = "Test2";
case Baz = "Test2\\a";
case FortyTwo = "42";
}

function zend_test_array_return(): array {}
Expand Down
7 changes: 6 additions & 1 deletion ext/zend_test/test_arginfo.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: 7b0abebae0b0eeea0f45dccb04759fceec1096e3 */
* Stub hash: 6d9ff0e2263a39420dd157206331a9e897d9e775 */

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_array_return, 0, 0, IS_ARRAY, 0)
ZEND_END_ARG_INFO()
Expand Down Expand Up @@ -417,6 +417,11 @@ static zend_class_entry *register_class_ZendTestStringEnum(void)
ZVAL_STR(&enum_case_Baz_value, enum_case_Baz_value_str);
zend_enum_add_case_cstr(class_entry, "Baz", &enum_case_Baz_value);

zval enum_case_FortyTwo_value;
zend_string *enum_case_FortyTwo_value_str = zend_string_init("42", sizeof("42") - 1, 1);
ZVAL_STR(&enum_case_FortyTwo_value, enum_case_FortyTwo_value_str);
zend_enum_add_case_cstr(class_entry, "FortyTwo", &enum_case_FortyTwo_value);

return class_entry;
}

Expand Down