Skip to content

Fix #81992: SplFixedArray::setSize() causes use-after-free #11959

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
wants to merge 3 commits into from
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
22 changes: 21 additions & 1 deletion ext/spl/spl_fixedarray.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ typedef struct _spl_fixedarray {
zval *elements;
/* True if this was modified after the last call to get_properties or the hash table wasn't rebuilt. */
bool should_rebuild_properties;
/* If positive, it's a resize within a resize and the value gives the desired size. If -1, it's not. */
zend_long cached_resize;
} spl_fixedarray;

typedef struct _spl_fixedarray_methods {
Expand Down Expand Up @@ -117,6 +119,7 @@ static void spl_fixedarray_init(spl_fixedarray *array, zend_long size)
} else {
spl_fixedarray_default_ctor(array);
}
array->cached_resize = -1;
}

/* Copies the range [begin, end) into the fixedarray, beginning at `offset`.
Expand Down Expand Up @@ -148,6 +151,7 @@ static void spl_fixedarray_copy_ctor(spl_fixedarray *to, spl_fixedarray *from)
*/
static void spl_fixedarray_dtor_range(spl_fixedarray *array, zend_long from, zend_long to)
{
array->size = from;
zval *begin = array->elements + from, *end = array->elements + to;
while (begin != end) {
zval_ptr_dtor(begin++);
Expand Down Expand Up @@ -184,19 +188,35 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size)
return;
}

if (UNEXPECTED(array->cached_resize >= 0)) {
/* We're already resizing, so just remember the desired size.
* The resize will happen later. */
array->cached_resize = size;
return;
}
array->cached_resize = size;

/* clearing the array */
if (size == 0) {
spl_fixedarray_dtor(array);
array->elements = NULL;
array->size = 0;
Copy link
Member

@iluuu1994 iluuu1994 Aug 14, 2023

Choose a reason for hiding this comment

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

Shouldn't this be set before spl_fixedarray_dtor to avoid the same issue on deallocation of the whole array? Edit: Ah, we're using size in there. Thinking about it, we might even do other shenanigans like modify the array in the destructor of released objects...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have a closer look, I'm currently fixing the case of calling resize within resize, still causing uaf :/ I think I have a solution for that though.
I'll have a deeper look at your comment after that.

} else if (size > array->size) {
array->elements = safe_erealloc(array->elements, size, sizeof(zval), 0);
spl_fixedarray_init_elems(array, array->size, size);
array->size = size;
} else { /* size < array->size */
/* Size set in spl_fixedarray_dtor_range() */
spl_fixedarray_dtor_range(array, size, array->size);
array->elements = erealloc(array->elements, sizeof(zval) * size);
}

array->size = size;
/* If resized within the destructor, take the last resize command and perform it */
zend_long cached_resize = array->cached_resize;
array->cached_resize = -1;
if (cached_resize != size) {
spl_fixedarray_resize(array, cached_resize);
}
}

static HashTable* spl_fixedarray_object_get_gc(zend_object *obj, zval **table, int *n)
Expand Down
32 changes: 32 additions & 0 deletions ext/spl/tests/bug81992.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--TEST--
Bug #81992 (SplFixedArray::setSize() causes use-after-free)
--FILE--
<?php
class InvalidDestructor {
public function __destruct() {
global $obj;
var_dump($obj[0]);
try {
var_dump($obj[2]);
} catch (Throwable $e) {
echo $e->getMessage(), "\n";
}
try {
var_dump($obj[4]);
} catch (Throwable $e) {
echo $e->getMessage(), "\n";
}
}
}

$obj = new SplFixedArray(5);
$obj[0] = str_repeat("A", 10);
$obj[2] = str_repeat('B', 10);
$obj[3] = new InvalidDestructor();
$obj[4] = str_repeat('C', 10);
$obj->setSize(2);
?>
--EXPECT--
string(10) "AAAAAAAAAA"
Index invalid or out of range
Index invalid or out of range
66 changes: 66 additions & 0 deletions ext/spl/tests/bug81992b.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
--TEST--
Bug #81992 (SplFixedArray::setSize() causes use-after-free) - setSize variation
--FILE--
<?php
class InvalidDestructor {
public function __construct(
private int $desiredSize,
private SplFixedArray $obj,
) {}

public function __destruct() {
echo "In destructor\n";
$this->obj->setSize($this->desiredSize);
echo "Destroyed, size is now still ", $this->obj->getSize(), "\n";
}
}

class DestructorLogger {
public function __construct(private int $id) {}

public function __destruct() {
echo "Destroyed the logger with id ", $this->id, "\n";
}
}

function test(int $desiredSize) {
$obj = new SplFixedArray(5);
$obj[0] = str_repeat("A", 10);
$obj[1] = new DestructorLogger(1);
$obj[2] = str_repeat('B', 10);
$obj[3] = new InvalidDestructor($desiredSize, $obj);
$obj[4] = new DestructorLogger(4);
$obj->setSize(2);
echo "Size is now ", $obj->getSize(), "\n";
echo "Done\n";
}

echo "--- Smaller size test ---\n";
test(1);
echo "--- Equal size test ---\n";
test(2);
echo "--- Larger size test ---\n";
test(10);
?>
--EXPECT--
--- Smaller size test ---
In destructor
Destroyed, size is now still 2
Destroyed the logger with id 4
Destroyed the logger with id 1
Size is now 1
Done
--- Equal size test ---
In destructor
Destroyed, size is now still 2
Destroyed the logger with id 4
Size is now 2
Done
Destroyed the logger with id 1
--- Larger size test ---
In destructor
Destroyed, size is now still 2
Destroyed the logger with id 4
Size is now 10
Done
Destroyed the logger with id 1