Skip to content

Make array_pad's $length warning less confusing#10149

Merged
arnaud-lb merged 2 commits intophp:masterfrom
ndossche:array_pad-error
Jan 14, 2023
Merged

Make array_pad's $length warning less confusing#10149
arnaud-lb merged 2 commits intophp:masterfrom
ndossche:array_pad-error

Conversation

@ndossche
Copy link
Member

@ndossche ndossche commented Dec 21, 2022 •
edited
Loading

The error message was wrong; it is possible to use a larger length. The error message should be clear it's about how much padding is required.

Valid example with length greater than 1048576:

<?php
var_dump(array_pad(array_fill(0, 1048576 * 2), 1048576 * 3 - 1, 0));
?>

If we were to remove the - 1 in the above code snippet such that the required padding length becomes too large, it would be especially confusing for developers using the array_pad function.

This patch also removes a redundant check on pad_size_abs: it can never be negative since it is the absolute value of pad_size.

EDIT: I changed the code to check the new array length instead of having an arbtrary limit. This also simplifies the error message.

Note: I targeted the master branch because I'm not sure how much confusion is considered a bug.

mvorisek
mvorisek reviewed Dec 21, 2022
View reviewed changes
Girgias
Girgias reviewed Dec 22, 2022
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly sure of the wording, but this is definitely an improvement (also removes a duplicate computation).

cmb69
cmb69 reviewed Dec 22, 2022
View reviewed changes
@arnaud-lb
Copy link
Member

arnaud-lb commented Dec 23, 2022

This patch also removes a redundant check on pad_size_abs: it can never be negative since it is the absolute value of pad_size.

Actually this can happen due to undefined behavior, when the result of abs() can not be represented in the return type.

With ZEND_ABS(), and assuming two's complement, this happens with pad_size=INTMAX_MIN only. If sizeof(intmax_t) > sizeof(zend_long), the conversion is implementation defined as well.

We could check that pad_size > LONG_MIN before calling ZEND_ABS().

We should add a test with length=PHP_INT_MIN.

@ndossche
Copy link
Member Author

ndossche commented Dec 23, 2022

This patch also removes a redundant check on pad_size_abs: it can never be negative since it is the absolute value of pad_size.

Actually this can happen due to undefined behavior, when the result of abs() can not be represented in the return type.

Right. My compiler actually optimizes away the check because of the UB. So on my system the old code didn't actually perform the pad_size_abs<0 check at all.

With ZEND_ABS(), and assuming two's complement, this happens with pad_size=INTMAX_MIN only. If sizeof(intmax_t) > sizeof(zend_long), the conversion is implementation defined as well.

We could check that pad_size > LONG_MIN before calling ZEND_ABS().

We should add a test with length=PHP_INT_MIN.

I can take a look at testing this soon-ish.

@ndossche
Copy link
Member Author

ndossche commented Dec 26, 2022

I removed the arbitrary limit, and perform the check first on pad_size now.
As HT_MAX_SIZE is small enough, there are no overflow concerns anymore with ZEND_ABS or with negation, so the new code is quite simple.

@cmb69
Copy link
Member

cmb69 commented Dec 30, 2022

I removed the arbitrary limit, and perform the check first on pad_size now.

That looks reasonable, but I wonder why that 1048576 limit had been chosen. It has been introduced by 4b343a0, but the commit message is not helpful.

@arnaud-lb
Copy link
Member

arnaud-lb commented Jan 6, 2023

Found the original commit: 7e8039e, and I think a related internals post: https://marc.info/?l=php-internals&m=104919385614541&w=2, but no details about the crash or the fix, except that it's an arbitrarily chosen limit.

The implementation of array_pad back then has nothing in common with today's.

@arnaud-lb
Copy link
Member

arnaud-lb commented Jan 6, 2023

From reading the code of array_pad, it seems safe as long as HT_MAX_SIZE has the right value. Unfortunately, trying to create an array with nearly HT_MAX_SIZE elements will cause some assertion failures in zend_hash.c due to HT_SIZE_TO_MASK() evaluating to 0.

Also, HT_MAX_SIZE is a confusing name: zend_hash.c checks that the size is strictly lower than HT_MAX_SIZE.

This was referenced Jan 6, 2023
Closed
Merged
@arnaud-lb
Copy link
Member

arnaud-lb commented Jan 13, 2023

HT_MAX_SIZE now has the right value, and zend_hash allows to create arrays with a size <= HT_MAX_SIZE.

@nielsdos could you adjust array_pad_too_large_padding.phpt? Checking with just PHP_INT_MIN and PHP_INT_MAX is enough.

I will merge after that.

@ndossche
Remove array_pad's arbitrary length restriction
e3f2351 
The error message was wrong; it *is* possible to use a larger length.
Furthermore, there is an arbitrary restriction on the new array's
length.
Fix both by checking the length against HT_MAX_SIZE.
@ndossche
Split array_pad() tests and add more edge case tests
56f3ef4
@ndossche
Copy link
Member Author

ndossche commented Jan 13, 2023

HT_MAX_SIZE now has the right value, and zend_hash allows to create arrays with a size <= HT_MAX_SIZE.

@nielsdos could you adjust array_pad_too_large_padding.phpt? Checking with just PHP_INT_MIN and PHP_INT_MAX is enough.

I will merge after that.

Thank you, I've rebased and updated the test.

@arnaud-lb arnaud-lb merged commit 6ab5038 into php:master Jan 14, 2023
@arnaud-lb
Copy link
Member

arnaud-lb commented Jan 14, 2023

Thank you!

@ndossche
Copy link
Member Author

ndossche commented Jan 14, 2023

Thank you for the merge!
Just a quick question: doesn't this also need an entry in UPGRADING, because the documentation needs to be updated? (https://www.php.net/manual/en/function.array-pad mentions the 1048576 limit)

arnaud-lb added a commit that referenced this pull request Jan 20, 2023
@arnaud-lb
[ci skip] UPGRADING
db6840b 
GH-10149
@arnaud-lb
Copy link
Member

arnaud-lb commented Jan 20, 2023

Indeed! I've just added an UPGRADING entry

@ndossche
Copy link
Member Author

ndossche commented Jan 20, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arnaud-lb arnaud-lb arnaud-lb left review comments

@cmb69 cmb69 cmb69 left review comments

@Girgias Girgias Girgias left review comments

+1 more reviewer

@mvorisek mvorisek mvorisek left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Extension: standard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ndossche @arnaud-lb @cmb69 @mvorisek @Girgias