Page MenuHomeFreeBSD

libmd: Only define SHA256_Transform_c when using the ARM64 ifunc.
ClosedPublic

Authored by jhb on Sep 13 2021, 6:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 7:22 AM
Unknown Object (File)
Mon, Mar 4, 8:08 AM
Unknown Object (File)
Feb 25 2024, 5:35 PM
Unknown Object (File)
Feb 8 2024, 8:41 PM
Unknown Object (File)
Dec 25 2023, 5:57 AM
Unknown Object (File)
Dec 20 2023, 6:12 AM
Unknown Object (File)
Nov 22 2023, 2:50 AM
Unknown Object (File)
Nov 9 2023, 7:25 AM

Details

Summary

GCC 9 doesn't define a SHA256_Transform symbol when the stub just wraps
SHA256_Transform_c resulting in an undefined symbol for
_libmd_SHA256_Transform in libmd.so.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 41499
Build 38388: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Sep 13 2021, 6:25 PM

This is definitely a workaround, and I don't fully understand why the tail call messes up this way when using GCC.

I don't think I understand the problem. SHA256_Transform is static, it should never be visible outside the object.

sys/crypto/sha2/sha256c.c
397

This reference is not static however.

sys/crypto/sha2/sha256c.c
397

Oh I see. That's still a bit fragile, even if it happens to work. The correct way to fix that is to mark any static symbols referenced by inline assembly via strings as __used, which will ensure it gets emitted.

397

(Though this refactoring is perhaps *also* a better way to write the original code, not sure, it's shorter, though requires two places to know about the ifdef).

sys/crypto/sha2/sha256c.c
397

So is __used the better long term fix? I could maybe have a wrapper macro that only maps to __used when WEAK_REFS is defined (WEAK_REFS_USED or some such) and then tag all the static functions with weak references with that?

Was reminded of this revision as Adrian reported this exact symptom on IRC

sys/crypto/sha2/sha256c.c
397

__used is both necessary and sufficient. Whether or not you prefer the other refactoring is a separate issue.

clang has also seems to exhibit the same behavior at -O1 or -O2 WITH_ASAN, as reported by @koobs.

Thanks for tagging @kevans. The *SAN link failures and environment/build configuration are here: https://lists.freebsd.org/archives/freebsd-current/2021-September/000488.html

Was there a conclusion to this? I have a similar sha512 implementation so would like to include the fix there.

I haven't gone back and tried the __used variant, but I could just land this perhaps. I think I do slightly prefer this version with not having the _c variant for !ARM64.

I think that's ok. It's still technically wrong without __used, but it was like that before too, and in practice non-static functions like SHA256_Update can't inline the ifunc (and wouldn't want to inline the non-ifunc on non-arm64 due to its size and being called multiple times) so that symbol's unlikely to disappear in the assembly, even if there should be a __used on it (and any others that are static and used for __weak_reference, though I vaguely recall Transform being the only one).

For new code though please use __used from the start for any static function referenced by __weak_reference and then write the code however you like

I just encountered this again while trying to fix WITH_ASAN and WITH_UBSAN.
https://cirrus-ci.com/task/6650127776481280

--- all_subdir_lib ---
--- all_subdir_lib/libc ---
ld: error: /usr/obj/tmp/cirrus-ci-build/amd64.amd64/tmp/usr/lib/libmd.so: undefined reference to _libmd_SHA256_Transform [--no-allow-shlib-undefined]
cc: error: linker command failed with exit code 1 (use -v to see invocation)
*** [h_hash.full] Error code 1

(See D32805 and PR260099 for other ASAN/UBSAN changes needed.)

This revision is now accepted and ready to land.Dec 8 2021, 7:47 PM