Skip to content

Cast big endian byte shuffling to uint #15988

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
Sep 24, 2024

Conversation

NattyNarwhal
Copy link
Member

@NattyNarwhal NattyNarwhal commented Sep 22, 2024

This works, but UBSan running on a big endian platform (in this, ppc64) will complain that the ((uchar*)buffer)[n] is int, and shifting that could be weird. Since the value of what PHAR_GET_32 et al usually set are almost always unsigned, it makes sense to cast these as unsigned.

Fixes phar tests on a big endian system with UBSan enabled.

This works, but UBSan running on a big endian platform (in this, ppc64)
will complain that the ((uchar*)buffer)[n] is int, and shifting that
could be weird. Since the value of PHAR_GET_32 et al are almost always
unsigned, it makes sense to cast these as unsigned.

Fixes phar tests on a big endian system with UBSan enabled.
@NattyNarwhal
Copy link
Member Author

NattyNarwhal commented Sep 22, 2024

Wondering if it might be a good idea to centralize endianness mangling code in general; I saw pack.c has its own divergent one, as well as the LS SAPI.

(Also: Not sure what's with the ABI break tag? This should be OK and I didn't see anything in the labeller Oh, this)

@Girgias Girgias requested a review from nielsdos September 22, 2024 19:30
@cmb69
Copy link
Member

cmb69 commented Sep 22, 2024

Hmm, there is something wrong with the labeler; this PR is not supposed to be marked as ABI break; after all, ext/phar/phar.c is not a header (and phar doesn't publish any headers at all). Would someone know what's going on there?

@cmb69
Copy link
Member

cmb69 commented Sep 22, 2024

Wondering if it might be a good idea to centralize endianness mangling code in general;

Yes, that seems to be a very good idea. Relevant parts could be made available as APIs for extensions, too.

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.

This change looks good to me (but having a function like php_ifd_get32u() would be better).

@thesamesam
Copy link

Wondering if it might be a good idea to centralize endianness mangling code in general;

@NattyNarwhal Consider using https://github.com/projg2/portable-endianness which we maintain for exactly this (just copy it).

@cmb69 cmb69 removed the ABI break label Sep 23, 2024
@NattyNarwhal NattyNarwhal merged commit 6cef9d3 into php:PHP-8.2 Sep 24, 2024
7 of 8 checks passed
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.

4 participants