Fixed Small Bug in getBytesFromPhpConfigValue

[mm-tech]

Affected version

2.2.8

So I mistyped my Units in my php.ini. I wrote 50MB, not 50M, but had to look through the source code until I realized what my error was. While looking I found this code that tries to fix this kind of mistake. But it can't handle this mistake, because $units will always be one character, in case of 50M it will be "M" and in case of 50MB it will be "B". In my personal opinion it would be nice if MB units generated a warning message.

xenforo/src/XF/Util/Php.php:456

    protected static function getBytesFromPhpConfigValue($configParam)
[...]
        {
            $units = strtoupper(substr($configValue, -1));
            $value = intval($configValue);

            // note that KB, MB and GB are not actually valid in PHP config, but are frequently encountered

            switch ($units)
            {
                case 'K':
                case 'KB':
                    return $value * 1024;
                case 'M':
                case 'MB':
                    return $value * 1048576;
                case 'G':
                case 'GB':
                    return $value * 1073741824;
                default:
                    return $value;
            }
        }
    }

 

[Arantor]

Think you need something like:

$units = strtoupper(substr($configValue, -1));
if (strtoupper($units) == 'B')
{
    $units = strtoupper(substr($configValue, -2));
}

Otherwise the KB/MB/GB branches of the switch never match. This won’t prevent something weird like 1000000B being misread but logically it ends up working out the same as it falls through to the default case which is bytes anyway.

 

[TLDR]

imho, if at all, $units == 'B' should just throw an exception because you cannot assume "it has to be 'MB' if we find a 'B'" - it could also be KB or GB.
Also, I'm not sure how PHP handles an invalid configuration - in any way screaming loud could make such an error more visible. Masking it by assuming stuff would not help.

 

[Arantor]

That wasn’t what either the OP’s suggestion or my follow-up did - the OP suggestion tries to match KB or K (and the other combinations) but it’s only ever fed the last letter. Mine took that, and also added “if the last letter was a B, get the second to last letter as well) and feed that into the calculation” so it would handle both.

 

[mm-tech]

No sorry, I was not suggesting to add this, this is code in xenforo core and it is buggy so I reported it as a bug

 

[Arantor]

Oh, I thought you were suggesting a change because I was on mobile and couldn’t check the actual code...

 

[XF Bug Bot]

Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.12).

Change log:

More accurate way of parsing byte values from PHP config values.

There may be a delay before changes are rolled out to the XenForo Community.