Skip to content

Zend/zend_operators: fix casting underflowed unsigned to signed #8220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

MaxKellermann
Copy link
Contributor

Casting a huge unsigned value to signed is undefined behavior in C.
We need to cast to signed before subtracting.

@nikic
Copy link
Member

nikic commented Mar 20, 2022

Casting a huge unsigned value to signed is undefined behavior in C.

This is incorrect. It's implementation-defined behavior, and all compiler implementations supported by PHP agree on the behavior. In fact, it is your new implementation that has undefined behavior in case the subtraction overflows.

That being said, the current code is fishy in that size_t may be larger than int, which means that we might truncate the subtraction to a zero value, reporting equality of strings that are not equal in degenerate cases. As this code is only interested in returning a three-way comparison result, the absolute value of which should not matter, I would recommend adding a helper macro that compares two values and returns 0, 1 or -1. (Along the lines of ZEND_NORMALIZE_BOOL, but that one works on a subtraction result.)

@MaxKellermann
Copy link
Contributor Author

I rewrote the commit adding the macro ZEND_THREEWAY_COMPARE() as suggested, which uses two comparisons like ZEND_NORMALIZE_BOOL(), instead of explicit subtraction + cast.

@cmb69
Copy link
Member

cmb69 commented Mar 21, 2022

That changes requires to adjust some test expectations.

@MaxKellermann
Copy link
Contributor Author

That changes requires to adjust some test expectations.

Will do.

Casting a huge unsigned value to signed is implementation-defined
behavior in C.  By introducing the ZEND_THREEWAY_COMPARE() macro, we
can sidestep this integer overflow/underflow/casting problem.
@MaxKellermann
Copy link
Contributor Author

Another admendment: I had the parantheses around macro parameters wrong. (I hate macros, because they force me to write obscure code like that.)

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm fine with this, but it might make sense to add some info about the change to UPGRADING(.INTERNALS).

@ramsey
Copy link
Member

ramsey commented May 25, 2022

@MaxKellermann This is waiting on you to add info to UPGRADING.INTERNALS. Let me know if you need any help with that.

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

Successfully merging this pull request may close these issues.

6 participants