briansmith / ring

Safe, fast, small crypto using Rust
Other
3.69k stars 697 forks source link

Require __BYTE_ORDER__ to be defined for bi-endian target architectures. #1885

Closed briansmith closed 8 months ago

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c4742e0) 96.02% compared to head (39db572) 96.02%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1885 +/- ## ======================================= Coverage 96.02% 96.02% ======================================= Files 136 136 Lines 20776 20776 Branches 226 226 ======================================= Hits 19950 19950 Misses 792 792 Partials 34 34 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

briansmith commented 8 months ago

cc @uweigand @erichte-ibm @pkubaj. The basic idea here is to ensure that __BYTE_ORDER__ is defined properly since @uweigand's contribution to crypto/internal.h requires __BYTE_ORDER__ to be defined correctly, and to avoid using any other indication of (big-)endianness.

DavidHorton5339 commented 8 months ago

maybe off-topic but isn't mipsel little-endian?

MIPS is bi-endian, so although mipsel is the little-endian variant, OPENSSL_MIPS is insufficient on it own to uniquely identify the target elsewhere in the code. That's why __BYTE_ORDER__ is needed too, and the purpose of this PR is to make sure that is set for bi-endian target architectures where this uncertainty exists.

Perhaps it would be clearer to prepend 'Require __BYTE_ORDER__ to be defined for bi-endian target architectures.' to the comment block starting on line 45, and to replace 'big-endianness' with 'endianness'.

briansmith commented 8 months ago

replace 'big-endianness' with 'endianness'.

I implemented this suggestion.

briansmith commented 8 months ago

Thanks for reviewing this. I really appreciate it.