From fff017be8b8f4251b860dd27c11df7c5d6e3e63b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 20 Sep 2021 16:31:50 +0200 Subject: [PATCH 1/6] Fix #80663: Recursive SplFixedArray::setSize() may cause double-free We address the `::setSize(0)` case by setting `array->element = NULL` before we destroy the elements, and bail in this case out on re-entry into `spl_fixedarray_resize()`. --- ext/spl/spl_fixedarray.c | 12 +++++++----- ext/spl/tests/bug80663.phpt | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) create mode 100644 ext/spl/tests/bug80663.phpt diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 87457652ec1a8..2b12376ef5885 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -93,7 +93,7 @@ static void spl_fixedarray_init(spl_fixedarray *array, zend_long size) /* {{{ */ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* {{{ */ { - if (size == array->size) { + if (size == array->size || (size == 0 && array->elements == NULL)) { /* nothing to do */ return; } @@ -107,14 +107,16 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* {{{ /* clearing the array */ if (size == 0) { zend_long i; + zval *elements = array->elements; + + array->elements = NULL; for (i = 0; i < array->size; i++) { - zval_ptr_dtor(&(array->elements[i])); + zval_ptr_dtor(&(elements[i])); } - if (array->elements) { - efree(array->elements); - array->elements = NULL; + if (elements) { + efree(elements); } } else if (size > array->size) { array->elements = safe_erealloc(array->elements, size, sizeof(zval), 0); diff --git a/ext/spl/tests/bug80663.phpt b/ext/spl/tests/bug80663.phpt new file mode 100644 index 0000000000000..c8549f6921b6d --- /dev/null +++ b/ext/spl/tests/bug80663.phpt @@ -0,0 +1,20 @@ +--TEST-- +Bug #80663 (Recursive SplFixedArray::setSize() may cause double-free) +--FILE-- +setSize(0); + } catch (LogicException $ex) { + echo $ex->getMessage(); + } + } +} + +$obj = new SplFixedArray(1000); +$obj[0] = new InvalidDestructor(); +$obj->setSize(0); +?> +--EXPECT-- + From a5a3c720d48ca8f7413962f79ddfaffca08efbc5 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 20 Sep 2021 22:28:00 +0200 Subject: [PATCH 2/6] Remove superfluous check --- ext/spl/spl_fixedarray.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 2b12376ef5885..e6033e2dfa44b 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -115,9 +115,7 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* {{{ zval_ptr_dtor(&(elements[i])); } - if (elements) { - efree(elements); - } + efree(elements); } else if (size > array->size) { array->elements = safe_erealloc(array->elements, size, sizeof(zval), 0); memset(array->elements + array->size, '\0', sizeof(zval) * (size - array->size)); From e5f150f19f2810735a7ec55b1e22dc45e59c5ffd Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 20 Sep 2021 22:38:22 +0200 Subject: [PATCH 3/6] Move recursion prevention into the size == 0 branch --- ext/spl/spl_fixedarray.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index e6033e2dfa44b..fdbcdc88aef8f 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -93,7 +93,7 @@ static void spl_fixedarray_init(spl_fixedarray *array, zend_long size) /* {{{ */ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* {{{ */ { - if (size == array->size || (size == 0 && array->elements == NULL)) { + if (size == array->size) { /* nothing to do */ return; } @@ -106,16 +106,18 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* {{{ /* clearing the array */ if (size == 0) { - zend_long i; - zval *elements = array->elements; + if (array->elements != NULL) { + zend_long i; + zval *elements = array->elements; - array->elements = NULL; + array->elements = NULL; - for (i = 0; i < array->size; i++) { - zval_ptr_dtor(&(elements[i])); - } + for (i = 0; i < array->size; i++) { + zval_ptr_dtor(&(elements[i])); + } - efree(elements); + efree(elements); + } } else if (size > array->size) { array->elements = safe_erealloc(array->elements, size, sizeof(zval), 0); memset(array->elements + array->size, '\0', sizeof(zval) * (size - array->size)); From f29d2d88b7d468c08b9ccc020489815f536d0600 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 21 Sep 2021 14:32:00 +0200 Subject: [PATCH 4/6] Reset size before destroying Co-authored-by: Tyson Andre --- ext/spl/spl_fixedarray.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index fdbcdc88aef8f..4aa27a2c34ec3 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -111,12 +111,14 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* {{{ zval *elements = array->elements; array->elements = NULL; + array->size = 0; for (i = 0; i < array->size; i++) { zval_ptr_dtor(&(elements[i])); } efree(elements); + return; } } else if (size > array->size) { array->elements = safe_erealloc(array->elements, size, sizeof(zval), 0); From b0af852c07d611747de186ff7156ea687af2eecb Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 21 Sep 2021 15:20:26 +0200 Subject: [PATCH 5/6] Fix test case --- ext/spl/tests/bug80663.phpt | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ext/spl/tests/bug80663.phpt b/ext/spl/tests/bug80663.phpt index c8549f6921b6d..d9f122b869113 100644 --- a/ext/spl/tests/bug80663.phpt +++ b/ext/spl/tests/bug80663.phpt @@ -4,11 +4,7 @@ Bug #80663 (Recursive SplFixedArray::setSize() may cause double-free) setSize(0); - } catch (LogicException $ex) { - echo $ex->getMessage(); - } + $GLOBALS['obj']->setSize(0); } } @@ -16,5 +12,11 @@ $obj = new SplFixedArray(1000); $obj[0] = new InvalidDestructor(); $obj->setSize(0); ?> ---EXPECT-- +--EXPECTF-- +Notice: Undefined index: obj in %s on line %d +Fatal error: Uncaught Error: Call to a member function setSize() on null in %s:%d +Stack trace: +#0 [internal function]: InvalidDestructor->__destruct() +#1 {main} + thrown in %s on line %d From c851798e6bf9c6139c79ab287ca0372c2f84be61 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 21 Sep 2021 16:30:23 +0200 Subject: [PATCH 6/6] Fix actual issue and restore test expectation We actually need to destroy all elements. --- ext/spl/spl_fixedarray.c | 3 ++- ext/spl/tests/bug80663.phpt | 9 +-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 4aa27a2c34ec3..f4dff10786440 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -109,11 +109,12 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* {{{ if (array->elements != NULL) { zend_long i; zval *elements = array->elements; + zend_long old_size = array->size; array->elements = NULL; array->size = 0; - for (i = 0; i < array->size; i++) { + for (i = 0; i < old_size; i++) { zval_ptr_dtor(&(elements[i])); } diff --git a/ext/spl/tests/bug80663.phpt b/ext/spl/tests/bug80663.phpt index d9f122b869113..9e359cc60ef6d 100644 --- a/ext/spl/tests/bug80663.phpt +++ b/ext/spl/tests/bug80663.phpt @@ -12,11 +12,4 @@ $obj = new SplFixedArray(1000); $obj[0] = new InvalidDestructor(); $obj->setSize(0); ?> ---EXPECTF-- -Notice: Undefined index: obj in %s on line %d - -Fatal error: Uncaught Error: Call to a member function setSize() on null in %s:%d -Stack trace: -#0 [internal function]: InvalidDestructor->__destruct() -#1 {main} - thrown in %s on line %d +--EXPECT--