On 26/02/2024 06:25, Paul Eggert wrote:
On 2023-10-09 06:48, Pádraig Brady wrote:
An incremental patch attached to use xxhash128 (0.8.2)
shows a good improvement (note avx2 being used on this cpu):
xxhash128 is not a cryptographic hash function, so it doesn't attempt to
be random. Of course most people won't care - it's random "enough" - but
it would be a functionality change.
blake2 is cryptographic and would be random, but would bloat the 'sort'
executable with code that's hardly ever used.
To attack the problem in a more conservative way, I installed the
attached patch into coreutils. With it, 'sort -R' continues to use MD5
but on GNUish platforms 'sort' links libcrypto dynamically only if -R is
used (Bruno's suggestion). This doesn't significantly affect 'sort -R'
performance, and reduces the startup overhead of plain 'sort' to be what
it was before we started passing -lcrypto to gcc by default (in
coreutils 8.32).
I also toyed with changing MD5 to SHA512, but that hurt performance. For
what it's worth, although I tested with an Intel Xeon W-1350, which
supports SHA-NI as well as various AVX-512 options, I didn't see where
libcrypto (at least on Ubuntu 23.10, which has OpenSSL 3.0.10) takes
advantage of these special-purpose instructions.
I noticed that the dlopen() used an unversioned lib,
which generally won't work as that's what's available
at build time, not at run time.
Attached is a fix.
Note that also includes a comment for why we
check for SHA512 and not MD5 (to avoid deprecation warnings).
Also attached is a fix to a sc_tight_scope failure.
cheers,
Pádraig
From 09fed593048f744f5a76e23e58b232fd8922bf2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 26 Feb 2024 14:42:40 +0000
Subject: [PATCH 1/2] maint: avoid sc_tight_scope failure in sort.c
* cfg.mk: Exclude the ptr_MD5_* symbols added in
commit v9.4-130-g7f57ac2d2, as there is no way
to declare these static given they way they're defined.
---
cfg.mk | 3 +++
1 file changed, 3 insertions(+)
diff --git a/cfg.mk b/cfg.mk
index c0a131b2b..7da1a4b4a 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -927,6 +927,9 @@ _gl_TS_other_headers = $(srcdir)/src/*.h src/*.h
# Normally, the rule would detect its declaration, but that uses a
# different name, __clz_tab.
_gl_TS_unmarked_extern_vars = factor_clz_tab
+# Avoid tight_scope rule stating these should be static
+# as there is no way to achieve that with the way these are defined.
+_gl_TS_unmarked_extern_vars += ptr_MD5_.*
# Other tight_scope settings
_gl_TS_dir = .
_gl_TS_obj_files = src/*.$(OBJEXT)
--
2.43.0
From e8d92ab0aa57fa79afed52ecfebfb2d3a0f6c3ea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 26 Feb 2024 16:38:41 +0000
Subject: [PATCH 2/2] build: fix libcrypto version linked by sort at runtime
One should link the versioned lib at runtime,
and the unversioned lib at build time,
as the unversioned lib may not be installed,
and better couples the binary with the required version.
* configure.ac: Define LIBCRYPTO_SONAME, determined from
the test binary linked with -lcrypto. Also document
why we use SHA512() in the check, rather than MD5().
* src/sort.c (link_libcrypto): Use the versioned lib in dlopen().
---
configure.ac | 13 ++++++++++---
src/sort.c | 2 +-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac
index 043081b90..eb6c33aa4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -365,12 +365,16 @@ AS_CASE([$LIB_CRYPTO],
[AC_LANG_PROGRAM(
[[#include <dlfcn.h>
#include <openssl/sha.h>
+ /* Use SHA512 rather than MD5 here to avoid deprecation warnings.
+ So need to check HAVE_OPENSSL_MD5.. with DLOPEN_LIBCRYPTO. */
]],
[[return !(dlopen ("libcrypto.so", RTLD_LAZY | RTLD_GLOBAL)
&& SHA512 (0, 0, 0));]])],
[# readelf works with cross-builds; ldd works on more platforms.
- AS_CASE([`(readelf -d conftest$EXEEXT || ldd conftest$EXEEXT
- ) 2>/dev/null`],
+ LIBCRYPTO_SONAME="`(readelf -d conftest$EXEEXT || ldd conftest$EXEEXT
+ ) 2>/dev/null |
+ sed -n 's/.*\(libcrypto.so.[[.0-9]]*\).*/\1/p'`"
+ AS_CASE([$LIBCRYPTO_SONAME],
[*libcrypto*],
[utils_cv_dlopen_libcrypto=yes])])
LIBS=$saved_LIBS])
@@ -378,7 +382,10 @@ AS_CASE([$LIB_CRYPTO],
[yes],
[AC_DEFINE([DLOPEN_LIBCRYPTO], [1],
[Define to 1 if dlopen exists and libcrypto is
- linked dynamically.])])])
+ linked dynamically.])
+ AC_DEFINE_UNQUOTED([LIBCRYPTO_SONAME], ["$LIBCRYPTO_SONAME"],
+ [versioned libcrypto])
+ ])])
# macOS >= 10.12
AC_CHECK_FUNCS([fclonefileat])
diff --git a/src/sort.c b/src/sort.c
index cefe381bf..2d8324ca4 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -2125,7 +2125,7 @@ static void
link_libcrypto (void)
{
#if DLOPEN_LIBCRYPTO && HAVE_OPENSSL_MD5
- void *handle = dlopen ("libcrypto.so", RTLD_LAZY | RTLD_GLOBAL);
+ void *handle = dlopen (LIBCRYPTO_SONAME, RTLD_LAZY | RTLD_GLOBAL);
if (!handle)
link_failure ();
ptr_MD5_Init = symbol_address (handle, "MD5_Init");
--
2.43.0
