diff --git a/NEWS b/NEWS index 9807e07945df..7105b8d44f5b 100644 --- a/NEWS +++ b/NEWS @@ -97,6 +97,8 @@ PHP NEWS unsigned int timeout. (Weilin Du) . Fixed bug GH-22671 (assert.bail aborts the process when the assert callback throws an exception whose reporting re-throws). (iliaal) + . Fixed bug GH-22678 (Use-after-free in array_multisort() when the comparator + mutates the array being sorted). (azchin, iliaal) - Streams: . Fixed bug GH-21468 (Segfault in file_get_contents w/ a https URL diff --git a/ext/dom/obj_map.c b/ext/dom/obj_map.c index aa852c800473..4d6479e003f9 100644 --- a/ext/dom/obj_map.c +++ b/ext/dom/obj_map.c @@ -345,22 +345,20 @@ static void dom_map_get_by_class_name_item(dom_nnodemap_object *map, zend_long i if (nodep && index >= 0) { dom_node_idx_pair start_point = dom_obj_map_get_start_point(map, nodep, index); if (start_point.node) { - if (start_point.index > 0) { - /* Only start iteration at next point if we actually have an index to seek to. */ - itemnode = php_dom_next_in_tree_order(start_point.node, nodep); - } else { - itemnode = start_point.node; - } + itemnode = start_point.node; } else { itemnode = php_dom_first_child_of_container_node(nodep); + while (itemnode != NULL && !dom_matches_class_name(map, itemnode)) { + itemnode = php_dom_next_in_tree_order(itemnode, nodep); + } } - do { - --start_point.index; + for (; start_point.index > 0 && itemnode != NULL; --start_point.index) { + itemnode = php_dom_next_in_tree_order(itemnode, nodep); while (itemnode != NULL && !dom_matches_class_name(map, itemnode)) { itemnode = php_dom_next_in_tree_order(itemnode, nodep); } - } while (start_point.index > 0 && itemnode); + } } dom_ret_node_to_zobj(map, itemnode, return_value); if (itemnode) { diff --git a/ext/dom/tests/modern/common/Element_getElementsByClassName_item_random_access.phpt b/ext/dom/tests/modern/common/Element_getElementsByClassName_item_random_access.phpt new file mode 100644 index 000000000000..005ad45d3c25 --- /dev/null +++ b/ext/dom/tests/modern/common/Element_getElementsByClassName_item_random_access.phpt @@ -0,0 +1,84 @@ +--TEST-- +Dom\Element::getElementsByClassName() item() random access (cold and backwards) +--EXTENSIONS-- +dom +--FILE-- + + + +
+ +

+ + +HTML); + +$body = $dom->getElementsByTagName('body')->item(0); + +function id(?Dom\Element $e): string { + return $e === null ? 'NULL' : $e->id; +} + +echo "-- cold random access (fresh collection per call) --\n"; +foreach ([0, 1, 2, 3, 4, 5] as $i) { + $collection = $body->getElementsByClassName('x'); + echo "item($i) = ", id($collection->item($i)), "\n"; +} + +echo "-- backwards seek on one collection --\n"; +$collection = $body->getElementsByClassName('x'); +foreach ([4, 2, 0, 3, 1] as $i) { + echo "item($i) = ", id($collection->item($i)), "\n"; +} + +echo "-- item() seed then foreach --\n"; +$collection = $body->getElementsByClassName('x'); +$collection->item(3); +$ids = []; +foreach ($collection as $node) { + $ids[] = $node->id; +} +echo implode(" ", $ids), "\n"; + +echo "-- last-element idiom --\n"; +$collection = $body->getElementsByClassName('x'); +echo "length = ", $collection->length, ", last = ", id($collection->item($collection->length - 1)), "\n"; + +echo "-- live collection after mutation --\n"; +$collection = $body->getElementsByClassName('x'); +echo "item(1) = ", id($collection->item(1)), "\n"; +$dom->getElementById('E1')->remove(); +echo "item(1) = ", id($collection->item(1)), ", length = ", $collection->length, "\n"; + +?> +--EXPECT-- +-- cold random access (fresh collection per call) -- +item(0) = E0 +item(1) = E1 +item(2) = E2 +item(3) = E3 +item(4) = E4 +item(5) = NULL +-- backwards seek on one collection -- +item(4) = E4 +item(2) = E2 +item(0) = E0 +item(3) = E3 +item(1) = E1 +-- item() seed then foreach -- +E0 E1 E2 E3 E4 +-- last-element idiom -- +length = 5, last = E4 +-- live collection after mutation -- +item(1) = E1 +item(1) = E2, length = 4 diff --git a/ext/ftp/ftp.c b/ext/ftp/ftp.c index 2ea93f979683..0035e8508da1 100644 --- a/ext/ftp/ftp.c +++ b/ext/ftp/ftp.c @@ -817,6 +817,11 @@ bool ftp_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const size_ if (ftp == NULL) { return false; } + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return false; + } + ftp->in_use = true; if (!ftp_type(ftp, type)) { goto bail; } @@ -913,9 +918,11 @@ bool ftp_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const size_ goto bail; } + ftp->in_use = false; return true; bail: data_close(ftp); + ftp->in_use = false; return false; } @@ -1000,6 +1007,11 @@ bool ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream if (ftp == NULL) { return false; } + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return false; + } + ftp->in_use = true; if (!ftp_type(ftp, type)) { goto bail; } @@ -1040,9 +1052,11 @@ bool ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) { goto bail; } + ftp->in_use = false; return true; bail: data_close(ftp); + ftp->in_use = false; return false; } @@ -1053,6 +1067,11 @@ bool ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stre if (ftp == NULL) { return false; } + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return false; + } + ftp->in_use = true; if (!ftp_type(ftp, type)) { goto bail; } @@ -1080,9 +1099,11 @@ bool ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stre if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) { goto bail; } + ftp->in_use = false; return true; bail: data_close(ftp); + ftp->in_use = false; return false; } @@ -1939,6 +1960,10 @@ static char** ftp_genlist(ftpbuf_t *ftp, const char *cmd, const size_t cmd_len, char **entry; char *text; + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return NULL; + } if ((tmpstream = php_stream_fopen_tmpfile()) == NULL) { php_error_docref(NULL, E_WARNING, "Unable to create temporary file. Check permissions in temporary files directory."); @@ -2037,6 +2062,11 @@ int ftp_nb_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const siz return PHP_FTP_FAILED; } + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return PHP_FTP_FAILED; + } + if (ftp->data != NULL) { /* If there is a transfer in action, abort it. * If we don't, we get an invalid state and memory leaks when the new connection gets opened. */ @@ -2101,11 +2131,17 @@ int ftp_nb_continue_read(ftpbuf_t *ftp) data = ftp->data; + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return PHP_FTP_FAILED; + } + /* check if there is already more data */ if (!data_available(ftp, data->fd, false)) { return PHP_FTP_MOREDATA; } + ftp->in_use = true; type = ftp->type; lastch = ftp->lastch; @@ -2129,6 +2165,7 @@ int ftp_nb_continue_read(ftpbuf_t *ftp) } ftp->lastch = lastch; + ftp->in_use = false; return PHP_FTP_MOREDATA; } @@ -2143,9 +2180,11 @@ int ftp_nb_continue_read(ftpbuf_t *ftp) } ftp->nb = false; + ftp->in_use = false; return PHP_FTP_FINISHED; bail: ftp->nb = false; + ftp->in_use = false; data_close(ftp); return PHP_FTP_FAILED; } @@ -2158,6 +2197,10 @@ int ftp_nb_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_strea if (ftp == NULL) { return 0; } + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return PHP_FTP_FAILED; + } if (!ftp_type(ftp, type)) { goto bail; } @@ -2201,16 +2244,24 @@ int ftp_nb_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_strea int ftp_nb_continue_write(ftpbuf_t *ftp) { + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + return PHP_FTP_FAILED; + } + /* check if we can write more data */ if (!data_writeable(ftp, ftp->data->fd)) { return PHP_FTP_MOREDATA; } + ftp->in_use = true; + if (ftp_send_stream_to_data_socket(ftp, ftp->data, ftp->stream, ftp->type, true) != SUCCESS) { goto bail; } if (!php_stream_eof(ftp->stream)) { + ftp->in_use = false; return PHP_FTP_MOREDATA; } @@ -2220,9 +2271,11 @@ int ftp_nb_continue_write(ftpbuf_t *ftp) goto bail; } ftp->nb = false; + ftp->in_use = false; return PHP_FTP_FINISHED; bail: data_close(ftp); ftp->nb = false; + ftp->in_use = false; return PHP_FTP_FAILED; } diff --git a/ext/ftp/ftp.h b/ext/ftp/ftp.h index b336754c3f88..1c8d45460ca6 100644 --- a/ext/ftp/ftp.h +++ b/ext/ftp/ftp.h @@ -70,6 +70,7 @@ typedef struct ftpbuf databuf_t *data; /* Data connection for "nonblocking" transfers */ php_stream *stream; /* output stream for "nonblocking" transfers */ bool nb; /* "nonblocking" transfer in progress */ + bool in_use; /* engine transfer in progress; blocks re-entrant ftp_close */ char lastch; /* last char of previous call */ bool direction; /* recv = 0 / send = 1 */ bool closestream;/* close or not close stream */ diff --git a/ext/ftp/php_ftp.c b/ext/ftp/php_ftp.c index 501ce9bc68d7..23bd69b5d63f 100644 --- a/ext/ftp/php_ftp.c +++ b/ext/ftp/php_ftp.c @@ -650,6 +650,11 @@ PHP_FUNCTION(ftp_nb_fget) } /* configuration */ + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + RETURN_FALSE; + } + ftp->direction = 0; /* recv */ ftp->closestream = 0; /* do not close */ @@ -760,6 +765,10 @@ PHP_FUNCTION(ftp_nb_get) RETURN_THROWS(); } GET_FTPBUF(ftp, z_ftp); + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + RETURN_FALSE; + } XTYPE(xtype, mode); /* ignore autoresume if autoseek is switched off */ @@ -797,8 +806,8 @@ PHP_FUNCTION(ftp_nb_get) ftp->closestream = true; /* do close */ if ((ret = ftp_nb_get(ftp, outstream, remote, remote_len, xtype, resumepos)) == PHP_FTP_FAILED) { - php_stream_close(outstream); ftp->stream = NULL; + php_stream_close(outstream); VCWD_UNLINK(local); if (*ftp->inbuf) { php_error_docref(NULL, E_WARNING, "%s", ftp->inbuf); @@ -807,8 +816,8 @@ PHP_FUNCTION(ftp_nb_get) } if (ret == PHP_FTP_FINISHED){ - php_stream_close(outstream); ftp->stream = NULL; + php_stream_close(outstream); } RETURN_LONG(ret); @@ -946,6 +955,11 @@ PHP_FUNCTION(ftp_nb_fput) } /* configuration */ + if (ftp->in_use) { + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + RETURN_FALSE; + } + ftp->direction = true; /* send */ ftp->closestream = false; /* do not close */ @@ -1086,6 +1100,12 @@ PHP_FUNCTION(ftp_nb_put) } } + if (ftp->in_use) { + php_stream_close(instream); + php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use"); + RETURN_FALSE; + } + /* configuration */ ftp->direction = true; /* send */ ftp->closestream = true; /* do close */ @@ -1093,8 +1113,8 @@ PHP_FUNCTION(ftp_nb_put) ret = ftp_nb_put(ftp, remote, remote_len, instream, xtype, startpos); if (ret != PHP_FTP_MOREDATA) { - php_stream_close(instream); ftp->stream = NULL; + php_stream_close(instream); } if (ret == PHP_FTP_FAILED) { @@ -1229,6 +1249,10 @@ PHP_FUNCTION(ftp_close) obj = ftp_object_from_zend_object(Z_OBJ_P(z_ftp)); if (obj->ftp) { + if (obj->ftp->in_use) { + zend_throw_error(NULL, "Cannot close FTP\\Connection while a transfer is in progress"); + RETURN_THROWS(); + } success = ftp_quit(obj->ftp); ftp_close(obj->ftp); obj->ftp = NULL; diff --git a/ext/ftp/tests/ftp_close_during_transfer.phpt b/ext/ftp/tests/ftp_close_during_transfer.phpt new file mode 100644 index 000000000000..72e410603921 --- /dev/null +++ b/ext/ftp/tests/ftp_close_during_transfer.phpt @@ -0,0 +1,44 @@ +--TEST-- +ftp_close() from a stream wrapper during a transfer throws instead of freeing the connection +--EXTENSIONS-- +ftp +pcntl +--FILE-- +getMessage(), "\n"; +} + +ftp_close($ftp); +echo "closed\n"; +?> +--EXPECT-- +bool(true) +Cannot close FTP\Connection while a transfer is in progress +closed diff --git a/ext/ftp/tests/ftp_nb_close_during_transfer.phpt b/ext/ftp/tests/ftp_nb_close_during_transfer.phpt new file mode 100644 index 000000000000..702648f69592 --- /dev/null +++ b/ext/ftp/tests/ftp_nb_close_during_transfer.phpt @@ -0,0 +1,44 @@ +--TEST-- +ftp_close() from a stream wrapper during a non-blocking transfer throws instead of freeing the connection +--EXTENSIONS-- +ftp +pcntl +--FILE-- +getMessage(), "\n"; +} + +ftp_close($ftp); +echo "closed\n"; +?> +--EXPECT-- +bool(true) +Cannot close FTP\Connection while a transfer is in progress +closed diff --git a/ext/ftp/tests/ftp_nb_get_during_nb_transfer.phpt b/ext/ftp/tests/ftp_nb_get_during_nb_transfer.phpt new file mode 100644 index 000000000000..deb1698c77c5 --- /dev/null +++ b/ext/ftp/tests/ftp_nb_get_during_nb_transfer.phpt @@ -0,0 +1,44 @@ +--TEST-- +Re-entrant ftp_nb_get() from a stream wrapper during an active non-blocking ftp_nb_get() must not corrupt the outer transfer +--EXTENSIONS-- +ftp +pcntl +--FILE-- + +--EXPECT-- +bool(true) +bool(true) +closed diff --git a/ext/ftp/tests/ftp_nb_get_during_transfer.phpt b/ext/ftp/tests/ftp_nb_get_during_transfer.phpt new file mode 100644 index 000000000000..c7920496feb4 --- /dev/null +++ b/ext/ftp/tests/ftp_nb_get_during_transfer.phpt @@ -0,0 +1,40 @@ +--TEST-- +Re-entrant ftp_nb_get() from a stream wrapper during a blocking ftp_get() must not free the active data connection +--EXTENSIONS-- +ftp +pcntl +--FILE-- + +--EXPECT-- +bool(true) +bool(true) +closed diff --git a/ext/standard/array.c b/ext/standard/array.c index a233e6c4dcb5..2d1dd584906a 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -5963,6 +5963,7 @@ PHP_FUNCTION(array_multisort) { zval* args; zval** arrays; + HashTable** hashes; Bucket** indirect; uint32_t idx; HashTable* hash; @@ -6084,11 +6085,17 @@ PHP_FUNCTION(array_multisort) for (i = 0; i < array_size; i++) { indirect[i] = indirects + (i * (num_arrays + 1)); } + hashes = safe_emalloc(num_arrays, sizeof(HashTable *), 0); + for (i = 0; i < num_arrays; i++) { + hashes[i] = Z_ARRVAL_P(arrays[i]); + GC_ADDREF(hashes[i]); + HT_ALLOW_COW_VIOLATION(hashes[i]); + } for (i = 0; i < num_arrays; i++) { k = 0; - if (HT_IS_PACKED(Z_ARRVAL_P(arrays[i]))) { - zval *zv = Z_ARRVAL_P(arrays[i])->arPacked; - for (idx = 0; idx < Z_ARRVAL_P(arrays[i])->nNumUsed; idx++, zv++) { + if (HT_IS_PACKED(hashes[i])) { + zval *zv = hashes[i]->arPacked; + for (idx = 0; idx < hashes[i]->nNumUsed; idx++, zv++) { if (Z_TYPE_P(zv) == IS_UNDEF) continue; ZVAL_COPY_VALUE(&indirect[k][i].val, zv); indirect[k][i].h = idx; @@ -6096,8 +6103,8 @@ PHP_FUNCTION(array_multisort) k++; } } else { - Bucket *p = Z_ARRVAL_P(arrays[i])->arData; - for (idx = 0; idx < Z_ARRVAL_P(arrays[i])->nNumUsed; idx++, p++) { + Bucket *p = hashes[i]->arData; + for (idx = 0; idx < hashes[i]->nNumUsed; idx++, p++) { if (Z_TYPE(p->val) == IS_UNDEF) continue; indirect[k][i] = *p; k++; @@ -6117,7 +6124,7 @@ PHP_FUNCTION(array_multisort) /* Restructure the arrays based on sorted indirect - this is mostly taken from zend_hash_sort() function. */ for (i = 0; i < num_arrays; i++) { - hash = Z_ARRVAL_P(arrays[i]); + hash = hashes[i]; hash->nNumUsed = array_size; hash->nNextFreeElement = array_size; hash->nInternalPointer = 0; @@ -6146,6 +6153,14 @@ PHP_FUNCTION(array_multisort) RETVAL_TRUE; clean_up: + for (i = 0; i < num_arrays; i++) { + if (UNEXPECTED(GC_DELREF(hashes[i]) == 0)) { + zend_array_destroy(hashes[i]); + } else { + gc_check_possible_root((zend_refcounted *)hashes[i]); + } + } + efree(hashes); efree(indirects); efree(indirect); efree(func); diff --git a/ext/standard/tests/array/gh22678.phpt b/ext/standard/tests/array/gh22678.phpt new file mode 100644 index 000000000000..c1b71b592520 --- /dev/null +++ b/ext/standard/tests/array/gh22678.phpt @@ -0,0 +1,48 @@ +--TEST-- +GH-22678 (Use-after-free in array_multisort() when the comparator mutates the array) +--FILE-- + 'c', 3 => 'a', 1 => 'b']; +array_multisort($b, SORT_STRING); +echo "repacked: ", implode(',', $b), "\n"; + +class Boom { + public function __toString(): string { + throw new Exception("boom"); + } +} + +$c = [new Boom(), new Boom()]; +try { + array_multisort($c, SORT_STRING); +} catch (Exception $e) { + echo $e::class, ": ", $e->getMessage(), "\n"; +} +?> +--EXPECT-- +freed mid-sort: bool(true) +repacked: a,b,c +Exception: boom