Validate `memory_limit` option #56

leuchtdiode · 2024-03-17T20:10:26Z

Q	A
Bugfix	yes
BC Break	no

Description

Passing invalid options via MemoryOptions#setMemoryLimit or via MemoryOptions#__construct, the actual memory_limit option is not properly validated.

…ormalizing memoy value; Added tests for setMemoryOptions()

src/MemoryOptions.php

test/unit/MemoryOptionsTest.php

boesing · 2024-03-21T19:18:23Z

I am not really concerned about this change. This introduces handling for invalid memory_limit values.
I tested this on PHP 8.3 and even PHP itself is yelling at me:

→ php -dmemory_limit=M256 -i | grep memory_limit
Using php v8.3.2
PHP Warning:  Invalid "memory_limit" setting. Invalid quantity "M256": no valid leading digits, interpreting as "0" for backwards compatibility in Unknown on line 0
PHP Warning:  Failed to set memory limit to 0 bytes (Current memory usage is 2097152 bytes) in Unknown on line 0
Warning: Invalid "memory_limit" setting. Invalid quantity "M256": no valid leading digits, interpreting as "0" for backwards compatibility in Unknown on line 0
memory_limit => 128M => 128M

I would instead enhance the documentation to outline that in case that a string value, passed as memory_limit option to the adapter, should properly support PHPs internal memory declaration structure.

Do we have other opinions here? I mean, there was already quite some discussion.

leuchtdiode · 2024-03-21T20:20:21Z

I am not really concerned about this change. This introduces handling for invalid memory_limit values. I tested this on PHP 8.3 and even PHP itself is yelling at me:
→ php -dmemory_limit=M256 -i | grep memory_limit
Using php v8.3.2
PHP Warning:  Invalid "memory_limit" setting. Invalid quantity "M256": no valid leading digits, interpreting as "0" for backwards compatibility in Unknown on line 0
PHP Warning:  Failed to set memory limit to 0 bytes (Current memory usage is 2097152 bytes) in Unknown on line 0
Warning: Invalid "memory_limit" setting. Invalid quantity "M256": no valid leading digits, interpreting as "0" for backwards compatibility in Unknown on line 0
memory_limit => 128M => 128M
I would instead enhance the documentation to outline that in case that a string value, passed as memory_limit option to the adapter, should properly support PHPs internal memory declaration structure.

Do we have other opinions here? I mean, there was already quite some discussion.

I'm with you at this. Why would someone expect a format to work which is no standard anywhere?

boesing · 2024-03-21T20:25:51Z

I'm with you at this. Why would someone expect a format to work which is no standard anywhere?

But that is what you are trying to support with this PR, no?
At least from what I can see in the test, you want to allow M256 as a valid memory limit. If that is not what you try to achieve, why are these changes needed in the first place? 😅

leuchtdiode · 2024-03-21T20:28:39Z

I'm with you at this. Why would someone expect a format to work which is no standard anywhere?

But that is what you are trying to support with this PR, no? At least from what I can see in the test, you want to allow M256 as a valid memory limit. If that is not what you try to achieve, why are these changes needed in the first place? 😅

No, this is a misunderstanding. The only thing I wanted to fix is the usage of ini_get('memory_limit') instead of the given parameter $value in normalizeMemoryLimit. This PR just got out of control because the tests are failing now because of my change 😂 And it raised the question on my side if we should prevent these wrong values from coming in.

I just added the test to "show" that it is currently wrong because it does return 256 (bytes). M256 and G1024 would go out of the test when we found a solution. Sorry, this was confusing from my side to put the two values into the data set.

boesing · 2024-03-21T20:32:49Z

Okay, let me rephrase what we want to achieve with this PR:

we do want to fix normalizeMemoryLimit method so that the value passed to setMemoryLimit is validated instead of the value persisted in memory_limit ini setting
we do want to have a failing test for those memory sizes you accidentally added to the valid ones, i.e. M256 and G1024
we do want to keep some of the other unit tests which are actually quite nice to have, I'd love to keep these

Did I forgot something? Let me know!

Could you probably refactor this PR to have only those changes in place? Happy to merge that once thats done (maybe not today but definitely in the next days). That would be perfect. Thanks in advance for contributing this!

I just added the test to "show" that it is currently wrong because it does return 256 (bytes). M256 and G1024 would go out of the test when we found a solution. Sorry, this was confusing from my side to put the two values into the data set.

Ah I see, well that escalated quickly 😂 However, lets try to focus on the initial problem. I'd just add a proper failing test for the invalid values. That should outline that the memory limit is not validated as it should be. Then we can have the fix in place and are good to go.

leuchtdiode · 2024-03-21T20:35:36Z

Okay, let me rephrase what we want to achieve with this PR:

we do want to fix normalizeMemoryLimit method so that the value passed to setMemoryLimit is validated instead of the value persisted in memory_limit ini setting

we do want to have a failing test for those memory sizes you accidentally added to the valid ones, i.e. M256 and G1024

we do want to keep some of the other unit tests which are actually quite nice to have, I'd love to keep these

Did I forgot something? Let me know!

Could you probably refactor this PR to have only those changes in place? Happy to merge that once thats done (maybe not today but definitely in the next days). That would be perfect. Thanks in advance for contributing this!

Yes, that's correctly summarized.

Point 1: Already done.
Point 2: I will change
Point 3: Does anyone have a problem with the null check in normalizeMemoryLimit This would let the existing tests pass.

…i's memory limit as default value Signed-off-by: Alex Schloegl <alex@try2catch.com>

Signed-off-by: Alex Schloegl <alex@try2catch.com>

leuchtdiode · 2024-03-21T20:56:23Z

I adapted the things we discussed. Please have a look if that suits you. Maybe you can double check the regex which I had to adapt to disallow M128, G1024, etc.
That i hopefully did not oversee something :)

The code formatting makes me crazy. I have to check how to apply phpcs.xml in PHPStorm 😂

boesing · 2024-03-21T21:17:02Z

Does anyone have a problem with the null check in normalizeMemoryLimit This would let the existing tests pass.

🤚🏼
Can you point me to the test which is failing so that I can have a look into that one before merging?

boesing · 2024-03-21T21:21:33Z

src/MemoryOptions.php

     */
    protected function normalizeMemoryLimit($value)
    {
        if (is_numeric($value)) {
            return (int) $value;
        }

-        if (! preg_match('/(\-?\d+)\s*(\w*)/', ini_get('memory_limit'), $matches)) {
-            throw new Exception\InvalidArgumentException("Invalid  memory limit '{$value}'");
+        if (! preg_match('/^(\-?\d+)\s*(\w*)$/', $value, $matches)) {


I think I'm fine with this one. I would say lets remove the $ for now.
I still wonder why a negative numeric value would be okay here but who am I to judge this ancient code.
Most probably only @weierophinney or @marc-mabe are aware of that and thus lets keep it for now. Could be removed in next major.
I most probably keep that in mind until then.

negative numeric is for -1 = no limit

Yup, but thats how it worked before as well.
If s1 wants to set unlimited memory, thats fine in the current major as thats how it works since like forever.
I am open to discuss this in an RFC for the next major version, would you agree on that?

boesing

I think after dropping the $ in the regex, this PR is ready to get merged.

Signed-off-by: Alex Schloegl <alex@try2catch.com>

leuchtdiode · 2024-03-21T22:08:39Z

I think after dropping the $ in the regex, this PR is ready to get merged.

Done :)

Xerkus · 2024-03-21T22:12:51Z

Why $ is dropped from regex? it should match full string
oof. Regex had no full match constraints? it could match weirdest stuff... so much for input validation

boesing · 2024-03-22T19:34:58Z

Why $ is dropped from regex? it should match full string oof. Regex had no full match constraints? it could match weirdest stuff... so much for input validation

Because it was introduced in this patch. Passing something like 1000 K YALLA is parsed as 1000 and K. I am not even super happy with the ^ because you could have passed YALLA 1000 K YALLA and it worked. But I do not really think that this might introduce too much noise but something like 1000 K (see that whitespace at the end?) should still be parsable (for now).

I think in next major, we should do something like:

if (is_numeric($value)) {
    return (int) $value;
}

if (! preg_match('/^(?<value>\-?\d+)\s+?(?<unit>[GMK])?$/i', trim($value), $matches)) {
    throw new Exception\InvalidArgumentException(sprintf(
        'Invalid memory limit "%s" passed. Must be compatible with PHP shorthand bytes notation',
        $value,
    ));
}

$value (int) $matches['value'];
$unit = strtoupper($matches['unit']);

if ($value < -1) {
    throw new Exception\InvalidArgumentException('Provided memory limit must represent a value greater than or equal to `-1`');
}

return match ($unit) {
   'G' => $value * 1024 * 1024 * 1024,
   'M' => $value * 1024 * 1024,
   'K' => $value * 1024,
   default => $value,
};

Fine for every1?

Signed-off-by: Alex Schloegl <alex@try2catch.com>

leuchtdiode · 2024-03-22T19:37:52Z

Why $ is dropped from regex? it should match full string oof. Regex had no full match constraints? it could match weirdest stuff... so much for input validation

Because it was introduced in this patch. Passing something like 1000 K YALLA is parsed as 1000 and K. I am not even super happy with the ^ you could have passed YALLA 1000 K YALLA and it worked. But I do not really think that this might introduce too much noise but something like 1000 K (see that whitespace at the end?) should still be parsable.

I think in next major, we should do something like:
if (is_numeric($value)) {
    return (int) $value;
}

if (! preg_match('/^(?<value>\-?\d+)\s+?(?<unit>[GMK])?$/i', trim($value), $matches)) {
    throw new Exception\InvalidArgumentException(sprintf(
        'Invalid memory limit "%s" passed. Must be compatible with PHP shorthand bytes notation',
        $value,
    ));
}

$value (int) $matches['value'];
$unit = strtoupper($matches['unit']);

if ($value < -1) {
    throw new Exception\InvalidArgumentException('Provided memory limit must represent a value greater than or equal to `-1`');
}

return match ($unit) {
   'G' => $value * 1024 * 1024 * 1024,
   'M' => $value * 1024 * 1024,
   'K' => $value * 1024,
   default => $value,
};
Fine for every1?

Sounds good to me. I will add "-1" and -1 to the valid data set.

Signed-off-by: Alex Schloegl <alex@try2catch.com>

Bugfix: Use given parameter instead of ini_get('memory_limit') when n…

b63f510

…ormalizing memoy value; Added tests for setMemoryOptions()

Ocramius requested changes Mar 18, 2024

View reviewed changes

src/MemoryOptions.php Outdated Show resolved Hide resolved

Ocramius reviewed Mar 18, 2024

View reviewed changes

src/MemoryOptions.php Outdated Show resolved Hide resolved

leuchtdiode added 3 commits March 18, 2024 08:08

Fixed formatting issues

92d9c18

Psalm and phpcs fixes

b35df2f

Psalm and phpcs fixes laminas#2

65336e5

Xerkus reviewed Mar 18, 2024

View reviewed changes

src/MemoryOptions.php Outdated Show resolved Hide resolved

boesing reviewed Mar 21, 2024

View reviewed changes

src/MemoryOptions.php Show resolved Hide resolved

boesing reviewed Mar 21, 2024

View reviewed changes

test/unit/MemoryOptionsTest.php Show resolved Hide resolved

boesing reviewed Mar 21, 2024

View reviewed changes

test/unit/MemoryOptionsTest.php Outdated Show resolved Hide resolved

boesing linked an issue Mar 21, 2024 that may be closed by this pull request

memory_limit option is not properly validate when passed via MemoryOptions::__construct or set via MemoryOptions#setMemoryLimit #54

Open

boesing added the Enhancement label Mar 21, 2024

leuchtdiode added 2 commits March 21, 2024 21:53

Adapted regex to allow characters only at the end; Set half of the in…

4a62d30

…i's memory limit as default value Signed-off-by: Alex Schloegl <alex@try2catch.com>

Fixed code formatting

9d484a4

Signed-off-by: Alex Schloegl <alex@try2catch.com>

boesing reviewed Mar 21, 2024

View reviewed changes

boesing requested changes Mar 21, 2024

View reviewed changes

Dropped the $ at the end of the regex

4840493

Signed-off-by: Alex Schloegl <alex@try2catch.com>

boesing changed the title ~~Bugfix: Use given parameter instead of ini_get('memory_limit') when n…~~ Validate memory_limit option Mar 22, 2024

boesing self-assigned this Mar 22, 2024

Added "-1" and -1 to valid data set

79b5189

Signed-off-by: Alex Schloegl <alex@try2catch.com>

Fixed formatting

79e4e82

Signed-off-by: Alex Schloegl <alex@try2catch.com>

boesing added this to the 2.3.1 milestone Mar 22, 2024

boesing added Bug Something isn't working and removed Enhancement labels Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `memory_limit` option #56

Validate `memory_limit` option #56

leuchtdiode commented Mar 17, 2024 •

edited by boesing

boesing commented Mar 21, 2024 •

edited

leuchtdiode commented Mar 21, 2024

boesing commented Mar 21, 2024

leuchtdiode commented Mar 21, 2024 •

edited

boesing commented Mar 21, 2024 •

edited

leuchtdiode commented Mar 21, 2024 •

edited

leuchtdiode commented Mar 21, 2024 •

edited

boesing commented Mar 21, 2024 •

edited

boesing Mar 21, 2024

Xerkus Mar 21, 2024

boesing Mar 22, 2024

boesing left a comment

leuchtdiode commented Mar 21, 2024

Xerkus commented Mar 21, 2024 •

edited

boesing commented Mar 22, 2024 •

edited

leuchtdiode commented Mar 22, 2024

Validate memory_limit option #56

Are you sure you want to change the base?

Validate memory_limit option #56

Conversation

leuchtdiode commented Mar 17, 2024 • edited by boesing

Description

boesing commented Mar 21, 2024 • edited

leuchtdiode commented Mar 21, 2024

boesing commented Mar 21, 2024

leuchtdiode commented Mar 21, 2024 • edited

boesing commented Mar 21, 2024 • edited

leuchtdiode commented Mar 21, 2024 • edited

leuchtdiode commented Mar 21, 2024 • edited

boesing commented Mar 21, 2024 • edited

boesing Mar 21, 2024

Choose a reason for hiding this comment

Xerkus Mar 21, 2024

Choose a reason for hiding this comment

boesing Mar 22, 2024

Choose a reason for hiding this comment

boesing left a comment

Choose a reason for hiding this comment

leuchtdiode commented Mar 21, 2024

Xerkus commented Mar 21, 2024 • edited

boesing commented Mar 22, 2024 • edited

leuchtdiode commented Mar 22, 2024

Validate `memory_limit` option #56

Validate `memory_limit` option #56

leuchtdiode commented Mar 17, 2024 •

edited by boesing

boesing commented Mar 21, 2024 •

edited

leuchtdiode commented Mar 21, 2024 •

edited

boesing commented Mar 21, 2024 •

edited

leuchtdiode commented Mar 21, 2024 •

edited

leuchtdiode commented Mar 21, 2024 •

edited

boesing commented Mar 21, 2024 •

edited

Xerkus commented Mar 21, 2024 •

edited

boesing commented Mar 22, 2024 •

edited