Skip to content

Fix GH-20194: null offset deprecation not emitted for writes #20238

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

Jump to bottom
Merged
merged 7 commits into from
Oct 29, 2025
Merged

Fix GH-20194: null offset deprecation not emitted for writes #20238

merged 7 commits into from
Oct 29, 2025

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Oct 19, 2025

Based on the patch from @nielsdos and split into different commits to explain what affects what.

@alexandre-daubois
Copy link
Member

alexandre-daubois commented Oct 20, 2025

Not approving as it's still in draft, but looks good. Thanks for taking care of this!

@Girgias Girgias linked an issue Oct 20, 2025 that may be closed by this pull request
Using null as an array offset is not deprecated #20194
Closed
arnaud-lb
arnaud-lb reviewed Oct 20, 2025
View reviewed changes
@Girgias Girgias changed the base branch from master to PHP-8.5 October 20, 2025 16:00
@Girgias Girgias marked this pull request as ready for review October 21, 2025 12:24
arnaud-lb
arnaud-lb approved these changes Oct 22, 2025
View reviewed changes
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Looks good to me!

dstogov
dstogov reviewed Oct 27, 2025
View reviewed changes
Zend/zend_vm_def.h
Comment on lines 6281 to 6295
} else if (UNEXPECTED(Z_TYPE_P(offset) == IS_NULL)) {
zend_error(E_DEPRECATED, "Using null as an array offset is deprecated, use an empty string instead");
if (UNEXPECTED(EG(exception))) {
zval_ptr_dtor_nogc(expr_ptr);
HANDLE_EXCEPTION();
}
Copy link
Member

@dstogov dstogov Oct 27, 2025

Choose a reason for hiding this comment

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

You know, each new warning opens a way to modify some involved zval through the user error handler.
In this case, user error handler may probably unset() op1 and this may lead to use-after-free on attempt to add new element.

It's better to add related test(s) and fix problem at first place.

cc @iluuu1994 @nielsdos

Copy link
Member

@arnaud-lb arnaud-lb Oct 27, 2025

Choose a reason for hiding this comment

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

Indeed:

set_error_handler(function ($errno, $errstr) {
    var_dump($errstr);
    global $a;
    unset($a);
});

$a = new stdClass;
$b = [0, null => $a];

I usually spot those, but missed this one.

I hope we can come up with a general solution to avoid these issues (#20018 / #12805).

Copy link
Member Author

@Girgias Girgias Oct 29, 2025

Choose a reason for hiding this comment

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

Added a test case and fix.

@Girgias Girgias merged commit 9a1b8a7 into php:PHP-8.5 Oct 29, 2025
10 checks passed
@Girgias Girgias deleted the 8.5-null-offset branch October 29, 2025 18:36
Girgias added a commit that referenced this pull request Oct 29, 2025
@Girgias
Merge branch 'PHP-8.5'
0df31e5 
* PHP-8.5:
  Update NEWS for null deprecation bug fix
  Fix GH-20194: null offset deprecation not emitted for writes (#20238)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dstogov dstogov dstogov left review comments

@arnaud-lb arnaud-lb arnaud-lb approved these changes

@alexandre-daubois alexandre-daubois alexandre-daubois approved these changes

@ndossche ndossche Awaiting requested review from ndossche

Assignees

No one assigned

Labels

ABI break Category: Build System Category: Engine Category: Optimizer Extension: bcmath Extension: bz2 Extension: calendar Extension: com_dotnet Extension: curl Extension: date Extension: dba Extension: dom Extension: exif Extension: ffi Extension: filter Extension: ftp Extension: gd Extension: gmp Extension: hash Extension: iconv Extension: intl Extension: json Extension: ldap Extension: libxml Extension: mbstring Extension: mysqli Extension: mysqlnd Extension: odbc Extension: opcache Extension: openssl Extension: pcre Extension: pdo (core) Extension: pdo_dblib Extension: pdo_firebird Extension: pdo_mysql Extension: pdo_odbc Extension: pdo_pgsql Extension: pdo_sqlite Extension: pgsql Extension: phar Extension: random Extension: reflection Extension: session Extension: simplexml Extension: snmp Extension: soap Extension: sockets Extension: sodium Extension: spl Extension: sqlite3 Extension: standard Extension: sysvmsg Extension: sysvsem Extension: sysvshm Extension: tidy Extension: tokenizer Extension: uri Extension: xml Extension: xmlreader Extension: xmlwriter Extension: xsl Extension: zend_test Extension: zip Extension: zlib SAPI: phpdbg

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using null as an array offset is not deprecated

4 participants

@Girgias @alexandre-daubois @arnaud-lb @dstogov