Skip to content

crypt_sha256/crypt_sha512, fix build with more recent versions of cla… #8897

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

Closed

Conversation

devnexen
Copy link
Member

…ng (> 11).

clang picks up the address subtraction to null as UB, but the implementations
are based on glibc.

error: performing pointer subtraction with a null pointer has undefined behavior [-Werror,-Wnull-pointer-subtraction] if ((key - (char *) 0) % __alignof__ (uint64_t) != 0) {

@devnexen devnexen force-pushed the crypt_sha_modern_clang_build_fix branch 2 times, most recently from 0d6150d to aa8b9b9 Compare June 30, 2022 06:06
@devnexen devnexen requested a review from Girgias June 30, 2022 06:19
@Girgias
Copy link
Member

Girgias commented Jun 30, 2022

I noticed this warning when compiling with Clang but I'm not sure how to fix it and if pragmas are desirable, and because this is security related code I'm far from confident to touch it, so I suppose the pragmas are OK

@devnexen
Copy link
Member Author

Yes since it s well based on the glibc implementation, I did not want to touch it neither.

@cmb69
Copy link
Member

cmb69 commented Jun 30, 2022

Undefined behavior is a serious issue, and I don't think we should suppress such warnings, if they are legit, and I think in this case they are.

Instead of subtracting a NULL pointer, shouldn't we just cast to uintptr_t?

@devnexen
Copy link
Member Author

Undefined behavior is a serious issue, and I don't think we should suppress such warnings, if they are legit, and I think in this case they are.

Instead of subtracting a NULL pointer, shouldn't we just cast to uintptr_t?

It appears even tough the related glibc code had been like this forever (even now). Ok will give a try then.

… casting instead.

While at it fixing werror level on phpdbg too.
@devnexen devnexen force-pushed the crypt_sha_modern_clang_build_fix branch from aa8b9b9 to d83ec6c Compare June 30, 2022 17:21
@Girgias Girgias requested review from cmb69 and Girgias July 1, 2022 01:40
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.

Where the PHPDBG changes present in the previous PR as well?

@devnexen
Copy link
Member Author

devnexen commented Jul 1, 2022

Nope :-)

@devnexen devnexen closed this in b356986 Jul 1, 2022
devnexen added a commit to devnexen/php-src that referenced this pull request Jul 1, 2022
@cmb69
Copy link
Member

cmb69 commented Jul 4, 2022

@devnexen, there have been some issues with the branches, and apparently this fix is no longer in "master" now. I assume it should be applied there as well, but you had reverted. Do we need to re-apply?

@devnexen
Copy link
Member Author

devnexen commented Jul 4, 2022

Yes I redid a new PR just for master

devnexen added a commit that referenced this pull request Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants