Restrict allowed usages of $GLOBALS #6487

nikic · 2020-12-03T15:08:02Z

RFC: https://wiki.php.net/rfc/restrict_globals_usage

This implements the restrictions, but does not yet fully capitalize on them (though it does drop INDIRECT checks in a couple places).

TysonAndre

Some minor nits and questions.

I'm in favor of restricting uses of $GLOBALS as well to avoid the unexpected bugs that missing INDIRECT handling can cause when functions were passed $GLOBALS as an argument

Zend/zend_vm_def.h

Zend/tests/unset_cv10.phpt

ext/standard/array.c

chapeupreto · 2020-12-24T01:11:43Z

Zend/tests/restrict_globals/invalid_assign_list_ref.phpt

+
+?>
+--EXPECTF--
+Fatal error: Cannot assign reference to non referencable value in %s on line %d


Out of curiosity: it is referencable or referenceable (or both)?

As you can see here,

php-src/Zend/zend_vm_def.h

Line 2334 in 5bd7305

zend_error(E_NOTICE, "Attempting to set reference to non referenceable value");

the word referenceable is being used.

There are a few other places that use the referencable word.

If referenceable is the proper option, I can submit a PR that changes those files.

It seems that both spellings are accepted, but google prefers referenceable.

The function is still used by var.c in some places.

…OfGlobals sniff > Access to the $GLOBALS array is now subject to a number of restrictions. > Read and write access to individual array elements like $GLOBALS['var'] > continues to work as-is. Read-only access to the entire $GLOBALS array also > continues to be supported. However, write access to the entire $GLOBALS > array is no longer supported. For example, array_pop($GLOBALS) will result > in an error. Refs: * https://www.php.net/manual/en/migration81.incompatible.php#migration81.incompatible.core.globals-access * https://github.com/php/php-src/blob/28a1a6be0873a109cb02ba32784bf046b87a02e4/UPGRADING#L23-L29 * https://wiki.php.net/rfc/restrict_globals_usage * php/php-src#6487 This commit introduces a new sniff which attempts to find all code patterns affected by this change, as per the code samples highlighted in the RFC. * There will no longer be a recursive “GLOBALS” key in the `$GLOBALS` variable. * Writes to `$GLOBALS` taken as a whole will now generate a compile error, this includes list assignments and unsetting of `$GLOBALS`. * Passing `$GLOBALS` by reference will trigger a runtime `Error` exception. * Appending unnamed entries to the `$GLOBALS` array is no longer supported. * etc Notes: * There are two specific situations which constitute a behavioural change. I.e. the code will work in both PHP < 8.1 as well as PHP 8.1+, but the result of the code will be different. The sniff will flag those situations with a `warning` instead of an `error` and will only flag these when both PHP < 8.1 as well as PHP 8.1+ need to be supported (based on the provided `testVersion`). * There is one known false negative - when `$GLOBALS` with array access is used in a short list. Detecting whether a variable is used in a short list _from within the list_ is hard and potentially very token walking intensive. As this is very, very much an edge case, which would be rare to come across in real code, I have deemed it unwise to pursue detection of this. * The `extract($GLOBALS, EXTR_REFS);` code sample, which is taken literally from the RFC, will not in actual fact throw a fatal error in PHP 8.1. As it is explicitly highlighted in the RFC as code that will break, it will be treated the same as all other code where `$GLOBALS` is passed by reference and will be flagged with an `error`. * For non-PHP native functions, it is not possible to the detect whether `$GLOBALS` is being passed by reference. As things are, these will not be flagged, though it could be considered to flag them with a lower `severity` to allow for manual inspection. Includes unit tests. Includes XML docs.

…veWithGlobalsVar` sniff This change is a side-effect of the "Remove Indirect Modification of $GLOBALS" RFC, which removed the recursive `$GLOBALS['GLOBALS']` key from the `$GLOBALS` array. > Access to the $GLOBALS array is now subject to a number of restrictions. > Read and write access to individual array elements like $GLOBALS['var'] > continues to work as-is. Read-only access to the entire $GLOBALS array also > continues to be supported. However, write access to the entire $GLOBALS > array is no longer supported. For example, array_pop($GLOBALS) will result > in an error. Refs: * https://www.php.net/manual/en/migration81.incompatible.php#migration81.incompatible.core.globals-access * https://github.com/php/php-src/blob/28a1a6be0873a109cb02ba32784bf046b87a02e4/UPGRADING#L23-L29 * https://wiki.php.net/rfc/restrict_globals_usage * php/php-src#6487 Includes unit tests. Includes XML docs.

…OfGlobals sniff > Access to the $GLOBALS array is now subject to a number of restrictions. > Read and write access to individual array elements like $GLOBALS['var'] > continues to work as-is. Read-only access to the entire $GLOBALS array also > continues to be supported. However, write access to the entire $GLOBALS > array is no longer supported. For example, array_pop($GLOBALS) will result > in an error. Refs: * https://www.php.net/manual/en/migration81.incompatible.php#migration81.incompatible.core.globals-access * https://github.com/php/php-src/blob/28a1a6be0873a109cb02ba32784bf046b87a02e4/UPGRADING#L23-L29 * https://wiki.php.net/rfc/restrict_globals_usage * php/php-src#6487 This commit introduces a new sniff which attempts to find all code patterns affected by this change, as per the code samples highlighted in the RFC. * There will no longer be a recursive “GLOBALS” key in the `$GLOBALS` variable. * Writes to `$GLOBALS` taken as a whole will now generate a compile error, this includes list assignments and unsetting of `$GLOBALS`. * Passing `$GLOBALS` by reference will trigger a runtime `Error` exception. * Appending unnamed entries to the `$GLOBALS` array is no longer supported. * etc Notes: * There are two specific situations which constitute a behavioural change. I.e. the code will work in both PHP < 8.1 as well as PHP 8.1+, but the result of the code will be different. The sniff will flag those situations with a `warning` instead of an `error` and will only flag these when both PHP < 8.1 as well as PHP 8.1+ need to be supported (based on the provided `testVersion`). * There is one known false negative - when `$GLOBALS` with array access is used in a short list. Detecting whether a variable is used in a short list _from within the list_ is hard and potentially very token walking intensive. As this is very, very much an edge case, which would be rare to come across in real code, I have deemed it unwise to pursue detection of this. * The `extract($GLOBALS, EXTR_REFS);` code sample, which is taken literally from the RFC, will not in actual fact throw a fatal error in PHP 8.1. As it is explicitly highlighted in the RFC as code that will break, it will be treated the same as all other code where `$GLOBALS` is passed by reference and will be flagged with an `error`. * For non-PHP native functions, it is not possible to the detect whether `$GLOBALS` is being passed by reference. As things are, these will not be flagged, though it could be considered to flag them with a lower `severity` to allow for manual inspection. * For calls to PHP native functions in combination with named parameters, it is only possible to detect pass by reference if the parameter name hasn't changed between PHP versions. Unfortunately, it has for the two function calls used in the tests. To get round that, we'd need a complete list of all PHP functions which receive parameters by reference, the parameter names across PHP versions and the parameter positions. In my estimation, that wouldn't actually add much value as for code written using PHP 8.0+ syntax, we should generally be able to expect that PHPCS will also be run on PHP 8.0+, in which case, it's a non-issue. Includes unit tests. Includes XML docs.

nikic added the RFC label Dec 3, 2020

nikic added this to the PHP 8.1 milestone Dec 3, 2020

nikic force-pushed the restrict-globals branch from 11dc801 to b2b9a86 Compare December 4, 2020 08:57

TysonAndre reviewed Dec 5, 2020

View reviewed changes

Zend/zend_vm_def.h Show resolved Hide resolved

Zend/tests/unset_cv10.phpt Outdated Show resolved Hide resolved

ext/standard/array.c Outdated Show resolved Hide resolved

chapeupreto reviewed Dec 24, 2020

View reviewed changes

nikic added 4 commits January 6, 2021 12:07

Restrict allowed usages of $GLOBALS

5619f27

Remove some INDIRECT handling in VM

8856f92

Remove some INDIRECT handling in array functions

e89f220

Remove unnecessary zend_array_count() uses

a9a489c

The function is still used by var.c in some places.

nikic force-pushed the restrict-globals branch from b2b9a86 to a9a489c Compare January 6, 2021 11:11

php-pulls closed this in 3c68f38 Jan 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Restrict allowed usages of $GLOBALS #6487

Restrict allowed usages of $GLOBALS #6487

Uh oh!

nikic commented Dec 3, 2020 •

edited

Loading

Uh oh!

TysonAndre left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chapeupreto Dec 24, 2020 •

edited

Loading

Uh oh!

nikic Jan 6, 2021

Uh oh!

Uh oh!

Restrict allowed usages of $GLOBALS #6487

Restrict allowed usages of $GLOBALS #6487

Uh oh!

Conversation

nikic commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TysonAndre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chapeupreto Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic commented Dec 3, 2020 •

edited

Loading

chapeupreto Dec 24, 2020 •

edited

Loading