Make array_pad's $length warning less confusing

[nielsdos]

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.

[Girgias]

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

[arnaud-lb]

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.

[nielsdos]

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.

[nielsdos]

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]

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]

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]

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.

[arnaud-lb]

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.

[nielsdos]

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]

Thank you!

[nielsdos]

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]

Indeed! I've just added an UPGRADING entry

[nielsdos]

Thank you!