php / php-src

The PHP Interpreter
https://www.php.net
Other
38.16k stars 7.75k forks source link

phpinfo() accepts any integer value as parameter #15871

Open EmadAlmahdi opened 1 month ago

EmadAlmahdi commented 1 month ago

Description

Bug Report: phpinfo() Accepts Any Integer Value as Parameter

Issue Summary: The phpinfo() function in PHP accepts any integer as a parameter, even though it is supposed to only accept predefined constants as flags. This allows invalid flag values (e.g., phpinfo(9999);) to be passed without throwing any error or warning.

Expected Behavior: The phpinfo() function should validate the parameter to ensure that only valid flags (constants like INFO_GENERAL, INFO_CREDITS, etc.) are passed. Passing an invalid flag should throw an error (e.g., ValueError) or trigger a warning. Additionally, an enum could be used to ensure better handling and readability of these flags.

Current Behavior: Currently, calling phpinfo() with any arbitrary integer (e.g., phpinfo(9999);) does not produce an error and still generates output, which could lead to unexpected behavior.

Steps to Reproduce:

  1. Call phpinfo(9999); or any invalid integer.
  2. Observe that no error is thrown, and the function continues to execute.
// Example of unexpected behavior:
phpinfo(9999);

Suggested Fixes:

Flags Reference:

INFO_GENERAL      => 1
INFO_CREDITS      => 2
INFO_CONFIGURATION=> 4
INFO_MODULES      => 8
INFO_ENVIRONMENT  => 16
INFO_VARIABLES    => 32
INFO_LICENSE      => 64
INFO_ALL          => -1

Link to phpinfo() documentation

PHP Version

PHP 8.3.11

Operating System

Windows 11

devnexen commented 1 month ago

I see your point, IMHO it is tangential to a behavior change thus a feature request, but might be better if an "old timer" spins his point here cc @cmb69

damianwadley commented 1 month ago

Agreed that this is a request, not a bug.

The parameter is a bitmask. Any number is potentially valid. If you think that it should reject values < -1 or > 127 (let's pretend those are different) then that's the sort of idea that would affect tons of functions that also work with bitmasks, not just phpinfo(), and enforcing it with some functions but not others presents an annoyingly inconsistent experience.

  • Implement strict validation to check if the passed parameter is a valid constant (or combination of constants).

Constants like INFO_GENERAL are merely numbers. There is no way to validate that the developer used one of those by name.

  • Introduce an enum or another stricter typing system to define the allowed constants for better code clarity and maintainability.

Enums cannot be combined as bitmasks, and I can't think of a "stricter typing system" that wouldn't be way outside the scope of this issue.

EmadAlmahdi commented 1 month ago

Thank you both @devnexen @damianwadley for your input!

@damianwadley : Maybe there could be a middle-ground solution, such as logging a warning or info message when an out-of-range bitmask is used (for values < -1 or > 127, for example), without strictly enforcing or throwing an error. This might allow in keeping things backward-compatible.

cmb69 commented 1 month ago

Given that we introduced ValueErrors for arguments outside of the supported range for some (many?) functions during the last couple of minor PHP releases, it may make sense to also tackle this for parameters accepting bitmasks. It might even make sense to introduce some respective doc-block annotations for the stubs (although, to my knowledge, that has not been done for parameters accepting only a certain integer range).

Anyhow, this should likely be discussed on the internals mailing list. @EmadAlmahdi, feel free to start this discussion there! :)

iluuu1994 commented 1 month ago

Maybe there could be a middle-ground solution, such as logging a warning or info message when an out-of-range bitmask is used (for values < -1 or > 127, for example), without strictly enforcing or throwing an error. This might allow in keeping things backward-compatible.

Note there are still valid use-cases for falling outside this range. E.g. INFO_ALL & ~INFO_LICENSE.

cmb69 commented 1 month ago

https://github.com/php/php-src/blob/aa34950344f130862f6f3b194aa76f99d1c7221d/ext/standard/info.h#L33

https://github.com/php/php-src/blob/aa34950344f130862f6f3b194aa76f99d1c7221d/ext/standard/info.c#L1247

While the behavior is not undefined, that is confusing. And even for userland where INFO_ALL might be -1 or 4294967295.