Skip to content

Refactor SplFixedArray #7168

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 7 commits into from
Jun 18, 2021
Merged

Refactor SplFixedArray #7168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f7769c
Move spl_offset_convert_to_long() to spl_fixedarray.c
Girgias Jun 18, 2021
effb4b3
Refactor SplFixedArray offset handling
Girgias Jun 18, 2021
9ccde44
Use a proper Error to signal that [] cannot be used with SplFixedArray
Girgias Jun 18, 2021
17a32a9
Refactor SplFixedArray has_dimension helper
Girgias Jun 18, 2021
c2a90cc
Revert string offset semantics
Girgias Jun 18, 2021
7bd621c
Drop ZPP tests
Girgias Jun 18, 2021
cedc415
Add test checking offset handling
Girgias Jun 18, 2021
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
2 changes: 1 addition & 1 deletion ext/spl/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
PHP_NEW_EXTENSION(spl, php_spl.c spl_functions.c spl_engine.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c, no,, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
PHP_NEW_EXTENSION(spl, php_spl.c spl_functions.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c, no,, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
PHP_INSTALL_HEADERS([ext/spl], [php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h])
PHP_ADD_EXTENSION_DEP(spl, pcre, true)
PHP_ADD_EXTENSION_DEP(spl, standard, true)
Expand Down
2 changes: 1 addition & 1 deletion ext/spl/config.w32
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// vim:ft=javascript

EXTENSION("spl", "php_spl.c spl_functions.c spl_engine.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c", false /*never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
EXTENSION("spl", "php_spl.c spl_functions.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c", false /*never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
PHP_SPL="yes";
PHP_INSTALL_HEADERS("ext/spl", "php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h");
59 changes: 0 additions & 59 deletions ext/spl/spl_engine.c
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions ext/spl/spl_engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
#include "php_spl.h"
#include "zend_interfaces.h"

PHPAPI zend_long spl_offset_convert_to_long(zval *offset);

static inline void spl_instantiate_arg_ex1(zend_class_entry *pce, zval *retval, zval *arg1)
{
object_init_ex(retval, pce);
Expand Down
84 changes: 56 additions & 28 deletions ext/spl/spl_fixedarray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,24 +309,56 @@ static zend_object *spl_fixedarray_object_clone(zend_object *old_object)
return new_object;
}

static zend_long spl_offset_convert_to_long(zval *offset) /* {{{ */
{
try_again:
switch (Z_TYPE_P(offset)) {
case IS_STRING: {
zend_ulong index;
if (ZEND_HANDLE_NUMERIC(Z_STR_P(offset), index)) {
return (zend_long) index;
}
break;
}
case IS_DOUBLE:
return zend_dval_to_lval_safe(Z_DVAL_P(offset));
case IS_LONG:
return Z_LVAL_P(offset);
case IS_FALSE:
return 0;
case IS_TRUE:
return 1;
case IS_REFERENCE:
offset = Z_REFVAL_P(offset);
goto try_again;
case IS_RESOURCE:
zend_error(E_WARNING, "Resource ID#%d used as offset, casting to integer (%d)",
Z_RES_HANDLE_P(offset), Z_RES_HANDLE_P(offset));
return Z_RES_HANDLE_P(offset);
}

zend_type_error("Illegal offset type");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would including the offset type make sense here - it may make this error easier to debug

zend_type_error("Illegal offset type %s", zend_zval_type_name(offset);

It's used in a few places

php-src/ext/session/session.c
2515:                                           zend_type_error("%s(): Option \"%s\" must be of type string|int|bool, %s given",
2516-                                                   get_active_function_name(), ZSTR_VAL(str_idx), zend_zval_type_name(value)
2517-                                           );

return 0;
}

static zval *spl_fixedarray_object_read_dimension_helper(spl_fixedarray_object *intern, zval *offset)
{
zend_long index;

/* we have to return NULL on error here to avoid memleak because of
* ZE duplicating uninitialized_zval_ptr */
if (!offset) {
zend_throw_exception(spl_ce_RuntimeException, "Index invalid or out of range", 0);
zend_throw_error(NULL, "[] operator not supported for SplFixedArray");
return NULL;
}

if (Z_TYPE_P(offset) != IS_LONG) {
index = spl_offset_convert_to_long(offset);
} else {
index = Z_LVAL_P(offset);
index = spl_offset_convert_to_long(offset);
if (EG(exception)) {
return NULL;
}

if (index < 0 || index >= intern->array.size) {
// TODO Change error message and use OutOfBound SPL Exception?
zend_throw_exception(spl_ce_RuntimeException, "Index invalid or out of range", 0);
return NULL;
} else {
Expand Down Expand Up @@ -368,17 +400,17 @@ static void spl_fixedarray_object_write_dimension_helper(spl_fixedarray_object *

if (!offset) {
/* '$array[] = value' syntax is not supported */
zend_throw_exception(spl_ce_RuntimeException, "Index invalid or out of range", 0);
zend_throw_error(NULL, "[] operator not supported for SplFixedArray");
return;
}

if (Z_TYPE_P(offset) != IS_LONG) {
index = spl_offset_convert_to_long(offset);
} else {
index = Z_LVAL_P(offset);
index = spl_offset_convert_to_long(offset);
if (EG(exception)) {
return;
}

if (index < 0 || index >= intern->array.size) {
// TODO Change error message and use OutOfBound SPL Exception?
zend_throw_exception(spl_ce_RuntimeException, "Index invalid or out of range", 0);
return;
} else {
Expand Down Expand Up @@ -410,13 +442,13 @@ static void spl_fixedarray_object_unset_dimension_helper(spl_fixedarray_object *
{
zend_long index;

if (Z_TYPE_P(offset) != IS_LONG) {
index = spl_offset_convert_to_long(offset);
} else {
index = Z_LVAL_P(offset);
index = spl_offset_convert_to_long(offset);
if (EG(exception)) {
return;
}

if (index < 0 || index >= intern->array.size) {
// TODO Change error message and use OutOfBound SPL Exception?
zend_throw_exception(spl_ce_RuntimeException, "Index invalid or out of range", 0);
return;
} else {
Expand All @@ -439,28 +471,24 @@ static void spl_fixedarray_object_unset_dimension(zend_object *object, zval *off
spl_fixedarray_object_unset_dimension_helper(intern, offset);
}

static int spl_fixedarray_object_has_dimension_helper(spl_fixedarray_object *intern, zval *offset, int check_empty)
static bool spl_fixedarray_object_has_dimension_helper(spl_fixedarray_object *intern, zval *offset, bool check_empty)
{
zend_long index;
int retval;

if (Z_TYPE_P(offset) != IS_LONG) {
index = spl_offset_convert_to_long(offset);
} else {
index = Z_LVAL_P(offset);
index = spl_offset_convert_to_long(offset);
if (EG(exception)) {
return false;
}

if (index < 0 || index >= intern->array.size) {
retval = 0;
} else {
if (check_empty) {
retval = zend_is_true(&intern->array.elements[index]);
} else {
retval = Z_TYPE(intern->array.elements[index]) != IS_NULL;
}
return false;
}

if (check_empty) {
return zend_is_true(&intern->array.elements[index]);
}

return retval;
return Z_TYPE(intern->array.elements[index]) != IS_NULL;
}

static int spl_fixedarray_object_has_dimension(zend_object *object, zval *offset, int check_empty)
Expand Down
14 changes: 7 additions & 7 deletions ext/spl/tests/fixedarray_001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@ $a = new SplFixedArray(0);
try {
$a[0] = "value1";
} catch (RuntimeException $e) {
echo "Exception: ".$e->getMessage()."\n";
echo $e::class, ': ', $e->getMessage(), "\n";
}
try {
var_dump($a["asdf"]);
} catch (RuntimeException $e) {
echo "Exception: ".$e->getMessage()."\n";
} catch (\TypeError $e) {
echo $e::class, ': ', $e->getMessage(), "\n";
}
try {
unset($a[-1]);
} catch (RuntimeException $e) {
echo "Exception: ".$e->getMessage()."\n";
echo $e::class, ': ', $e->getMessage(), "\n";
}
$a->setSize(10);

Expand Down Expand Up @@ -45,9 +45,9 @@ $a[0] = "valueNew";
var_dump($b[0]);
?>
--EXPECT--
Exception: Index invalid or out of range
Exception: Index invalid or out of range
Exception: Index invalid or out of range
RuntimeException: Index invalid or out of range
TypeError: Illegal offset type
RuntimeException: Index invalid or out of range
string(6) "value0"
string(6) "value2"
string(6) "value3"
Expand Down
14 changes: 7 additions & 7 deletions ext/spl/tests/fixedarray_002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,17 @@ $a = new A;
try {
$a[0] = "value1";
} catch (RuntimeException $e) {
echo "Exception: ".$e->getMessage()."\n";
echo $e::class, ': ', $e->getMessage(), "\n";
}
try {
var_dump($a["asdf"]);
} catch (RuntimeException $e) {
echo "Exception: ".$e->getMessage()."\n";
} catch (\TypeError $e) {
echo $e::class, ': ', $e->getMessage(), "\n";
}
try {
unset($a[-1]);
} catch (RuntimeException $e) {
echo "Exception: ".$e->getMessage()."\n";
echo $e::class, ': ', $e->getMessage(), "\n";
}
$a->setSize(10);

Expand All @@ -69,11 +69,11 @@ var_dump(count($a), $a->getSize(), count($a) == $a->getSize());
?>
--EXPECT--
A::offsetSet
Exception: Index invalid or out of range
RuntimeException: Index invalid or out of range
A::offsetGet
Exception: Index invalid or out of range
TypeError: Illegal offset type
A::offsetUnset
Exception: Index invalid or out of range
RuntimeException: Index invalid or out of range
A::offsetSet
A::offsetSet
A::offsetSet
Expand Down
Loading