wp_convert_hr_to_bytes() report correct byte sizes for php.ini "shorthand" values

[dmsnell]

Resolves #17725

When wp_convert_hr_to_bytes() was introduced in [4388] it provided a simplified mechanism to parse the values returned by functions like ini_get() which represent byte sizes. The over-simplified approach has led to issues in that function reporting the wrong byte sizes for various php.ini directives, leading to confusing problems such as uploading files that are rejected improperly or accepted improperly.

In this patch we're porting the parser from PHP's own source (which has remained stable for decades and probably can't change without major breakage) in order to more accurately reflect the values it uses when it reads those configurations.

Unfortunately PHP doesn't offer a mechanism to read its own internal value for these fields and a 100% port is extremely cumbersome (at best) due to the different ways that PHP and C handle signed integer overflow. These differences should only appear when supplying discouraged/invalid values to the system anyway, and PHP warns that in these situations things are likely to break anyway.

Over the years this function has been modified a couple of times in ways that this patch reverts:

• [38013] introduced a PHP_INT_MAX limit in a way that coerces hexadecimal and octal integer representations to decimal.
• [35325] replaced the hard-coded byte size with overwritable constants but if there were any occasion for someone to change those constants in wp-config.php then we would actually want to preserve the hard-coded values in wp_convert_hr_to_bytes() since that function refers to code inside of PHP, not inside of WordPress.
• The original code from [4388] looks for the presence of the suffixes anywhere within the value string and prioritizes g over m over k whereas PHP only looks at the last character in the input string (this is something that 17725.3.diff got right). This can cause unexpected parses, such as with 14gmk when WordPress interprets it as 14GiB but PHP interprets it as 14KiB.

Further we do acknowledge the mismatch between PHP's definition of "gigabyte"/"megabyte"/"kilobyte" being factors of 1024 apart from each other and the standard of being 1000. WordPress follows PHP's convention so this is simply noted in the function and preserved.

This patch introduces new behaviors which might seem unexpected or wrong. It's important to consider that this function exists because PHP doesn't expose the values it parses from the php.ini directives. Therefore it's job in WordPress can be considered to do as best as it can to represent what's really happening inside of PHP; this may not match our intuition about what PHP should be doing. To that end the over-simplified code for the past 16 years has misreported many plausible-looking values like 100MB (which PHP interprets as 100 bytes but WordPress thinks is 100 MiB).

Testing

In order to fully verify the updated code we have to understand PHP's interpretation of the php.ini directive values. One way to do this is to set a value, upload_max_size for instance, in any number of the possible configurable places and then make repeated uploads to see if it's rightfully accepted or rejected. This is cumbersome.

An alternative approach is to compile PHP locally with added instrumentation; this is the approach taken in preparing this PR. The following patch will report three values every time a "Long" value is parsed from a php.ini directive: the shorthand value being parsed, the bound long value before applying the magnitude suffix, and the possibly-overflowed value derived from applying the possible g, m, and k suffixes.

Zend/zend_operators.c

@@ -164 +164 @@
                                 break;
                 }
         }
+
+        printf("zend_atol( \"%s\" ) = %lld : %lld\n", str, ZEND_STRTOL(str, NULL, 0), retval);
+
         return (zend_long) retval;
 }
 /* }}} */

For example, a sampling of values run through PHP produces this output.

zend_atol( "0" ) = 0 : 0
zend_atol( "0g" ) = 0 : 0
zend_atol( "1g" ) = 1 : 1073741824
zend_atol( "3G" ) = 3 : 3221225472
zend_atol( "3mg" ) = 3 : 3221225472
zend_atol( "3km" ) = 3 : 3145728
zend_atol( "boat" ) = 0 : 0
zend_atol( "-14k" ) = -14 : -14336
zend_atol( "-14chairsg" ) = -14 : -15032385536
zend_atol( "9223372036854775807" ) = 9223372036854775807 : 9223372036854775807
zend_atol( "9223372036854775807g" ) = 9223372036854775807 : -1073741824
zend_atol( "9223372036854775808" ) = 9223372036854775807 : 9223372036854775807
zend_atol( "0xt" ) = 0 : 0
zend_atol( "0x5teak_and_egg" ) = 5 : 5368709120

[dmsnell]

Refactors wp_convert_hr_to_bytes() to accurately report values parsed internally in PHP.

[SergeyBiryukov]

Thanks for the patch!

Replying to dmsnell:

[35325] replaced the hard-coded byte size with overwritable constants but if there were any occasion for someone to change those constants in wp-config.php then we would actually want to preserve the hard-coded values in wp_convert_hr_to_bytes() since that function refers to code inside of PHP, not inside of WordPress.

Just noting that these constants are defined unconditionally in wp_initial_constants(), and it is not possible to change them in wp-config.php without getting PHP warnings, so the comment that they are redefinable seems inaccurate. I think it's safe to continue using them here, as it is their intended purpose to be used across all of core for better readability, instead of "magic numbers".

[costdev]

Patch updated. Single line comments converted to // format. Multiline comments converted to /* * */ format. WPCS fixes.

[costdev]

I have updated the initial patch with some tidying up.

The patch causes two errors and one failure in the tests.

Errors:

• 2x Trying to access array offset on value of type int when an integer is passed. $value should be cast to string.

Failure:

• Test value: '128m ' (note the trailing whitespace)
• Expected result: 134217728
• Actual result: 128

See tests/phpunit/tests/load/wpConvertHrToBytes.php at line 34 for the datasets.
Run vendor/bin/phpunit --filter Tests_Load_wpConvertHrToBytes to run the tests locally.

[dd32]

I absolutely hate the idea of writing PHP in the C style, but ultimately love the reason for doing so.

However, I have some concerns, specifically that this is a bit over the top for what it's trying to do.

My notes:

• GB_IN_BYTES can technically be changed, but if someone does that they deserve it.
• The leading whitespace checks aren't working, as it's using single-quoted strings, so looking for a literal '\t' 2-character string
• Isn't written in PHP coding style
• I don't think it needs to mirror PHP's parsing for a specific config

55635.3.diff​ is my take on the original issue, I haven't run it against the unit tests, but I think it'll resolve the underlying intention.
You can see the evolution in this gist: ​https://gist.github.com/dd32/b6d757bf26f7e417621076fe06925ea5/revisions where I took 55635.diff​ and slowly migrated it back to PHP, before coming to the conclusion that inval( $s, 0 ) is almost the same, albeit with additional binary notation support (which PHP ignores for ini settings, but that's fine IMHO)

It's worth noting, that just because PHP parses it one way, it's not necessarily strictly required that wp_convert_hr_to_bytes() should handle those extreme edge-cases.. but there's some obvious improvements that can be made here.

Also, to save some time, here's the PHP intval underlying C code I've used for reference: ​https://github.com/php/php-src/blob/b5db594fd277464104fce814d22f0b2207d6502d/ext/standard/type.c#L142-L199 which is ultimately passed through the C function ​strtol().

Additional edit: If you want to test the PHP parsing, you can use php -d display_errors=1 -d memory_limit=0x123m -r='echo ini_get("memory_limit"); $t=" "; while($t.=$t);' and consider the size of 305135616 bytes exhausted number to be it's byte interpretation (In this case, 291MB)

[dd32]

With less PHP fatals :facepalm:

[dmsnell]

Thank you @SergeyBiryukov; I wasn't sure if those constants could be redefined but I figured there was a reasonable case for doing so, that being to avoid confusion with the way PHP and WordPress use the backwards definitions. That is, maybe you want your site to show a 400MB image as 400MB and not as 381 (because the computer will show it as 400, a phone will show it as that, etc…).

I get that it could be rare and we can blame someone for changing this value if their site breaks, but on the other hand I figured it was one of those things where it's just as simple to do it right and it was adding the abstraction that introduced the potential for a bug, why not just keep it simple and accurate. Again, maybe we want a different function if people don't agree on the purpose of wp_convert_hr_to_bytes(), but its sole purpose is reporting how many bytes are represented by a given php.ini directive, and that's actually what led me to discover this discrepancy, so my patch proposed doing that.

@costdev I'm definitely not up on WordPress coding conventions but did you see that your patch stripped away the PHPdoc information from my patch? With those docblock style comments various IDEs and tools do provide helpful inline documentation whereas without them that disappears. If we aren't allowing them (of which I see literally thousands in the existing code so I think we are) then maybe we should remove them entirely from this patch.

If it's something more superficial, such as the linter expected them to be multi-line comments I think it would be better to fix the tool or add the newlines because those comments aren't that useful if they're detached from the variables they describe.

Trying to access array offset on value of type int when an integer is passed. $value should be cast to string.

Is this an error from real code or possibly just from a test? We can always add extra runtime type-checking and casting, though it seems like the intention here is to only be called with data from ini_get() which only returns string values (or false, which I suppose would be good to guard against). I checked and I don't believe any code in Core should be passing an integer so maybe there's buggy test code out there doing that thinking that this function wants an integer?

Test value: '128m ' (note the trailing whitespace)
Expected result: 134217728
Actual result: 128

As noted in the patch description and in the patch itself I expect some existing assumptions to break because in this case we have an invalid test and I'll need to make sure to update the test to reflect that. WordPress has been reporting 128m as 128 MiB but PHP will have limited the actual size to 128 bytes and so it will reject basically every file uploaded and the errors will be confusing: "Your upload limit is 128 MB, your 15 KB file could not be uploaded because it's too big."

Thanks for pointing out the style and lint issues and also the test. It's my plan to add tests, but I couldn't get to it today.

Concerning the list of $digits do we have a policy for excluding the formatting rule? I personally found it more distracting as a large multi-line list and so am hoping we can exclude that array for the sake of not distracting what's going on in the rest of the function.

I absolutely hate the idea of writing PHP in the C style, but ultimately love the reason for doing so.

I love the feedback @dd32 :) By the way the goal wasn't to write in C style but to simply have WordPress agree with PHP for the values of these memory sizes. When I started my code looked much more like your patch but I had to keep unwrapping it. In my head I keep going back to the idea: this isn't about telling PHP what to do so much as it is to properly read the values of those configs, though I'm confused on why PHP doesn't expose a way to read them.

GB_IN_BYTES can technically be changed, but if someone does that they deserve it.

Is it worth punishing someone who makes a mistake, or punishing the users of their site for their mishap, when doing so requires adding a layer of abstraction which can introduce a bug that isn't present in the simpler code? Case in point, 55635.3.1.diff has two typos (BYES instead of BYTES); granted, they are there when pulling those constants back in after I removed them, but in older versions of PHP that would have returned the string MB_IN_BYES instead of the constant or instead of throwing an error.

The leading whitespace checks aren't working, as it's using single-quoted strings, so looking for a literal '\t' 2-character string

Very nice catch 🙇‍♂️. I did my local testing with spaces but not with tabs yet. It's my intention to add an illustrative test suite to this to demonstrate cases just like this.

Isn't written in PHP coding style

I'm happy to update things to match WordPress style. Is this more a comment about style rules or about it not reading like other PHP tends to look? While certain aspects of this are more complicated than using (int) or intval(), they are written because of quirks on one or both sides of PHP/WordPress and are there to avoid knowingly misreporting memory size values.

There's one other approach I considered which was to build the scalar value by joining the digits as we parse them and then use PHP casting or intval() to do the numeric parsing. That's still an option but I want to avoid using (int) or other approaches on the input value because (as you noted) it incorporates a different semantic for how it parses numbers. If the goal is to be generally accurate enough for common configurations I don't think this patch is necessary, but if the goal is to accurately reflect the configured value then I disagree that it's good enough to be right most of the time and wrong in a broad number of situations. I'm particularly sensitive to this context since it could be easy to craft particular values that don't show errors and which WordPress shows as being reasonable but for which PHP has a very different value. The broken test-case that failed is an example, where WordPress is off by a factor of a million and it's the difference between allowing fairly large uploads and practically disabling all uploads albeit with a very conflicted error message. Also different versions of PHP result in different behaviors for intval() and some of those changes are as recent as 7.1. For example, 1e6 is one byte, not as one million bytes as intval() on PHP 8.1 is reporting it.

A RegExp was another approach I considered but felt was starting to stuff too much complexity into the code and too divergent from what's going on in PHP under the hood, and brought in more overhead than we need. At least with this incantation there's a general match between the purpose of the function and the algorithm taken to parse the config values. Should PHP decide to change this for some reason it should remain fairly straightforward to update the WordPress side and not have to rethink abstractions or new concepts introduced to cover the style differences.

I don't think it needs to mirror PHP's parsing for a specific config

Noted: I disagree :) Again, if we don't particularly care about the accuracy I don't see the point in fixing a small number of additional cases while leaving a very wide door for very misleading parses. Obviously I too have a subjective threshold because I've left the overflow cases out of this. Those cases seem a bit different though for these reasons:

• The PHP code involves some amount of undefined behavior and so it may not be possible to be generally accurate in those cases; a fix on my machine may not also fix a build with another compiler on another architecture or system or OS.
• The PHP code issues a warning against using these incorrect values.
• Writing the PHP to simulate the integer overflow was giving me quite the headache and I think it would involve quite a bit more complexity than the rest of this patch, which is mostly scanning through the string character by character in a single pass and pulling out values of interest.

We actually know that the digit parsing won't overflow because we have a defined behavior in strtol(). If there's no final g, m, or k value then I believe this should be a 100% match. We should be able to easily detect if an overflow should occur, but we don't have a way to respond to that that's clearly right (and while it seems reasonable to return 0 in such cases that also is more of a feeling than something that corresponds to the reality of the system).

Additional edit: If you want to test the PHP parsing

Nice! Obviously more helpful for smaller values than ones like 9223372036854775807 (64-bit max integer) but doesn't require compiling PHP.

---

Obviously I'll need to fix the bugs mentioned above. Thank you all for sharing your feedback here. Hope you don't mind the wall of text; it was my intention to share the context behind why I wrote what I did and the reasoning leading my thoughts.

[dmsnell]

Incorporates some style feedback and checks for false input.

[dmsnell]

@dd32 another quick thing: I considered moving this function into a new file such as wp-includes/php-compat.php but ultimately chose not to because that would involve adding a new file and then needing to require it inside load.php. That's still an option and possibly a way to isolate its more nuanced behavior from the more general WordPress code in the rest of the file. It could also add a nice way to separate some of the more cluttered parts of this function into separate helpers. If you think it's worth doing that I'd be happy to do so, but so far figured folks would prefer leaving this as-is in place since it doesn't require that level of separation yet.

[costdev]

Hi @dmsnell, I did remove the PHPDoc data from the single-line comments based on the ​Docs Standards. However, I'm not opposed to their existence in the code since /** @var exists in 722 locations elsewhere in Core.

Is this an error from real code or possibly just from a test?

This seems to only be in the test suite, appearing almost immediately, so it might exist in the test suite's bootstrap.

We can always add extra runtime type-checking and casting, though it seems like the intention here is to only be called with data from ini_get() which only returns string values (or false, which I suppose would be good to guard against). I checked and I don't believe any code in Core should be passing an integer so maybe there's buggy test code out there doing that thinking that this function wants an integer?

It's possible, and the function is documented to receive (string) $value. However, without the guard in place, there's the potential that a number of the ​440 plugins out there may experience some breakages, depending on how they're using it. A cursory glance shows that in many cases, they seem to be correctly passing a string.

Concerning the list of $digits do we have a policy for excluding the formatting rule? I personally found it more distracting as a large multi-line list and so am hoping we can exclude that array for the sake of not distracting what's going on in the rest of the function.

There are phpcs:ignore comments in Core, but none for the rule in question:

WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine

Generally, it's discouraged to ignore rules, although I don't know of any strict rule against it. I don't personally find the multiline list distracting, but I appreciate that you do.

Given that these two shorter alternatives aren't as clear, or as performant, as the original patch or the multiline list, I think it's either a case of a multiline list or a phpcs:ignore comment. I defer to consensus on that should the array be in the final patch.

<?php
$digits = range( 0, 9 );
$digits = array_merge(
        $digits,
        array(
                'A' => 10,
                'a' => 10,
                'B' => 11,
                'b' => 11,
                'C' => 12,
                'c' => 12,
                'D' => 13,
                'd' => 13,
                'E' => 14,
                'e' => 14,
                'F' => 15,
                'f' => 15,
        )
);

// OR

$digits = array_merge(
    range( 0, 9 ),
    array_combine(
        array( 'A', 'a', 'B', 'b', 'C', 'c', 'D', 'd', 'E', 'e', 'F', 'f' ),
        array( 10, 10, 11, 11, 12, 12, 13, 13, 14, 14, 15, 15 )
    )
);

[pento]

This is a fun patch. 🙂

I think this change should go in a new function, rather than altering the behaviour of the existing function. While the original intention of wp_convert_hr_to_bytes() was for quickly parsing PHP INI values, I suspect that it's often used for converting any "human readable" sizes to their integer equivalent (​example).

I would argue that wp_convert_hr_to_bytes('1MB') is a reasonable usage of the function, and returns the expected (if technically incorrect) value, particularly when comparing it to the behaviour of the corresponding wp_convert_bytes_to_hr() function (since deprecated in favour of size_format(), but still relevant).

One other note, the function should support -1 (as an integer, not a string), as this is commonly used for the WP_MEMORY_LIMIT/WP_MAX_MEMORY_LIMIT constants.

[dmsnell]

@costdev there are examples in that linked docs standard to single-line docblock comments, so maybe I will undo my change that turned them into multi-line statements. that is, unless some tool is simply unaware of the guideline and naively replaces all single-line comments it sees into //. can you confirm if it's happening automatically or will it be okay if I move back from multi-line to single-line?

without the guard in place, there's the potential that a number of the 440 plugins out there may experience some breakages, depending on how they're using it. A cursory glance shows that in many cases, they seem to be correctly passing a string.

That's a great tool! I manually reviewed all of the search results and confirmed that none are passing anything but the strings from ini_get() or other string values. Most of those search results are copying the Core code into non-admin contexts, which tells me that there's more good reason to do what @pento suggests and put this in another file, possibly php-combat.php.

There were a few plugins that pass an optional defined string and so it's possible someone might define(DEFAULT_SIZE, 15) but there's no indication that this is happening. One plugin passes a default value of 100000kb which unfortunately won't be what they think it is (the plugin is supposed to enable large uploads but it will limit uploads to 100K if it falls back to the default value).

Generally, it's discouraged to ignore rules, although I don't know of any strict rule against it. I don't personally find the multiline list distracting, but I appreciate that you do.

I'll play around some with it. I'd ideally like to avoid solutions we know are adding unnecessary overhead just for the looks of the code (such as dynamically building the array or calling a function to get the value). I want this code to be lean since it's a fundamental-level operation, but at the same time we're not parsing these numbers frequently and hopefully this is never representative of the hot path, so some tradeoff in performance for readability is probably fine.

I think this change should go in a new function, rather than altering the behaviour of the existing function.

originally I did this and having you say that makes me more confident to do that again. before building the patch for this ticket I figured it would be better to make a smaller change.

While the original intention of wp_convert_hr_to_bytes() was for quickly parsing PHP INI values, I suspect that it's often used for converting any "human readable" sizes to their integer equivalent (example).

This is unfortunate and possibly we could let this function remain what it sounds like it does, but make it more robust over time. 10mb represents 10 MiB in Core, but 10kg represents 10 GiB, and 10gm represents 10 MiB again. It's fairly surprising (to me) how it parses numeric descriptions.

If we have something like wp_ini_byte_size() (or ​`ini_bytes()`) we can move all calls to wp_convert_hr_to_bytes() to that instead.

On that note would it be worth instead depreciating wp_convert_hr_to_bytes() in an attempt to remove any such confusion over its use? That confusion being mostly that it did do something but we would want people moving forward to not use this for parsing ini_get() values. "If you want ini byte sizes use wp_ini_bytes(); if you want counts from byte descriptions use wp_hr_bytes()" Or maybe size_parse() or size_bytes() to mirror the language of size_format()

One other note, the function should support -1 (as an integer, not a string), as this is commonly used for the WP_MEMORY_LIMIT/WP_MAX_MEMORY_LIMIT constants.

This and others (handling integer inputs) might be good and easy with a new file. I'm going to try and create a PR on wordpress-develop with this in mind and pick wp-includes/php-compat.php as the file name unless anyone has a better suggestion.

Thanks all; hopefully we can make this a bit smoother with auto test runs by moving to GitHub

[costdev]

there are examples in that linked docs standard to single-line docblock comments, so maybe I will undo my change that turned them into multi-line statements. that is, unless some tool is simply unaware of the guideline and naively replaces all single-line comments it sees into . can you confirm if it's happening automatically or will it be okay if I move back from multi-line to single-line?

• I'm not seeing any instance of a single line /** @var ... */ comment in the docs standards. Translator comments are a different context, as are the docblocks for requires/includes.
• The change from /** @var ... */ to // is not automatic.
• You can change the line back to /** @var ... */ format without a tool converting it to something else.

Thanks for taking a closer look at the plugin results!

I'll play around some with it. I'd ideally like to avoid solutions we know are adding unnecessary overhead just for the looks of the code (such as dynamically building the array or calling a function to get the value). I want this code to be lean since it's a fundamental-level operation, but at the same time we're not parsing these numbers frequently and hopefully this is never representative of the hot path, so some tradeoff in performance for readability is probably fine.

Per my previous comment, I suggest that we don't sacrifice performance due to disagreement with a rule in the coding standard, and instead either meet the coding standard, or ignore the rule if consensus moves in that direction.

However, IMO, all patches should meet the coding standard, and if exceptions are to be made, they should be made about a near-final patch. Otherwise we'd have a significant increase in the number of phpcs:ignore comments in Core if an individual contributor added an ignore comment and reviewers just happened to miss it. As this ticket still has a number of steps to go through, including looking at two different proposed solutions, I think we should focus on these first.

This PR for discussion of code and tests - see companion ticket

Trac ticket: https://core.trac.wordpress.org/ticket/55635

Resolves #17725 (https://core.trac.wordpress.org/ticket/17725)

When wp_convert_hr_to_bytes() was introduced in [4388] it provided a simplified
mechanism to parse the values returned by functions like ini_get() which
represent byte sizes. The over-simplified approach has led to issues in that
function reporting the wrong byte sizes for various php.ini directives, leading
to confusing problems such as uploading files that are rejected improperly or
accepted improperly.

In this patch we're porting the parser from PHP's own source (which has
remained stable for decades and probably can't change without major breakage)
in order to more accurately reflect the values it uses when it reads those
configurations. This is available in the new function wp_ini_bytes() found
inside of wp-includes/php-compat.php and loaded automatically in load.php.

Unfortunately PHP doesn't offer a mechanism to read its own internal value for
these fields and a 100% port is extremely cumbersome (at best) due to the
different ways that PHP and C handle signed integer overflow. These differences
should only appear when supplying discouraged/invalid values to the system
anyway, and PHP warns that in these situations things are likely to break
anyway.

I suppose that the compatibility check won't block this PR, but I heeded it's advice an in this PHP-compat module fill in the PHP_INT_MAX and PHP_INT_MIN constants. Since this is pulled in at the front of load.php I think that means we should be able to relax the lint rule, because as long as WordPress is running it should be defined.

[dmsnell]

I've updated the patch in the ​associated PR #2660. Changes include:

• renaming the proposed function to wp_ini_parse_quantity() to match the naming in the ​PHP PR #8454
• replacing use of wp_convert_hr_bytes() with wp_ini_parse_quantity()
• replacing math on parsed memory values with helper functions which handle the "no limit" cases

[dmsnell]

Updates from wordpress-develop: use of new function in Core code

[dmsnell]

I've updated (55635.0.3.diff) the associated patch in ​#2660 again and would be interested in moving forward here.

What's different/new?

• ini_parse_quantity() is merged in PHP 8.2, which is ​schedule for release on Nov. 24
• there's a new set of functions for comparing these values: wp_ini_lesser_quantity(), wp_ini_greater_quantity(), and wp_ini_quantity_cmp(). these are valuable because we often want to retain the string value of the set INI directive while knowing if it's larger or smaller than another.
• I've updated some core code to use the new functionality. while some cases required a fair bit of work to understand what core is doing, I believe that the updated code is much easier to follow.

Can you let me know if I've overlooked any feedback?
Can you identify any blockers to this issue?
What is important to you before merging this?

P.S. I know there's still discussion about back-porting the internal PHP parsing routine. Unless someone feels really strongly about that I'd like to keep it in because it flows from the purpose of these updates that WordPress should agree with PHP on what these values mean. Being off by a factor of a million in normal cases is something that seems worth avoiding.