-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix Bug #75284 sha3 is not supported on bigendian machine #2789
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
Conversation
fc3141f
to
b10cf6a
Compare
Fedora scratch build with this patch applied (+ 7c83579) Ok, on bigendian
Ok on little endian (no warning)
|
Will probably do the opposite later
P.S. or not... so merge-up from 7.1 will fail and raise dev attention about different implementation... |
@remicollet, did you try the config.m4 only diff I posted to the bug report? I think we can fallback on the provided 32bit algo when we encounter a 64bit BE machine. |
@sgolemon thanks for digging on this Yes, your proposal seems much more simpler, but using 32 bits code on 64 bits... seems strange... and indeed doesn't work: From Test build https://koji.fedoraproject.org/koji/taskinfo?taskID=22202631 FAILED TEST SUMMARYhash_copy() via clone [ext/hash/tests/hash-clone.phpt] TEST FAILURE: ../ext/hash/tests/hash-clone.diff -- Well... switching to old 7.1 slow algo is not better.... (perhaps even worst)
So I really need to book a bigendian server to try to fix this issue in 7.0+ and then come back to this 7.2+ one... |
Ok, 1 step done, slow 7.1 algo fixed in fa78afa |
4af3668
to
95a5a08
Compare
95a5a08
to
d3ff2d3
Compare
Patch simplified, CI ok. @sgolemon Any other good idea ? |
@remicollet Damn, that's unfortunate, I was hoping the 32bit path would just be less efficient. Definitely wanted someone with hardware to try though, cause the above was a easily a possible outcome. My only other "good idea" is to fix the library upstream. Let me see if I can pull that off (I actually can get a 64bit BE machine, just not on short notice) and if we get an upstream fix in place then we can have a later discussion as to whether or not we should pull it back onto 7.2 |
EXT_HASH_SHA3_SOURCES="$SHA3_OPT_SRC $SHA3_DIR/KeccakHash.c $SHA3_DIR/KeccakSponge.c" | ||
PHP_HASH_CFLAGS="-I@ext_srcdir@/$SHA3_DIR -DKeccakP200_excluded -DKeccakP400_excluded -DKeccakP800_excluded" | ||
|
||
if test $ac_cv_c_bigendian_php = yes; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This over-pessimizes big endian. 32bit BE works fine on 32bitBE archs (well, I assume it does based on scanning the code). BE is a minority regardless, so it's probably fine, but I'd be inclined to only go slow route for the broken path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have 64-bit bigendian target.... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgolemon I think the safer way for now if to apply this PR (at least before RC4, so I can build 7.2 in Fedora), then we can work on it later... but I indeed think bigendian is minority, and 32-bit another minority... 1% of 1%... (we still have some for RHEL-6... 7 years old... and 7.2 already mostly unsupported there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with that. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldnt drop 32bit support be a great thing for php 7.3/8.0 (for the whole php-src codebase)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@staabm I don't think we can drop 32-bit support, mostly because of ARM computers which are still quite common (aarch64 is not yet widely used)
Merged |
This PR restore the old slow algo, which will be only used on bigendian computer.