Subject: bug#64058: [PATCH] wc: Fix crashes due to incomplete AVX2 enumeration

From: Paul Eggert <[email-masked]>
Date: Wed, 14 Jun 2023 15:04:57 -0700

On 6/14/23 03:46, Pádraig Brady wrote:

> Paul you removed the "avx" check from cksum.c. Was that intended?

No, it's a typo I introduced. Thanks for catching that. Fixed in the 
first attached patch.

While looking into this I noticed a couple of other cleanups, fixed in 
the other attached patches.

I installed these into coreutils on Savannah.

From 7814596fa91a07fb2f1d0972f93f26de8a4ad547 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 14 Jun 2023 14:13:35 -0700
Subject: [PATCH 1/3] cksum: fix bug in check for cksum_pclmul
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes a typo in the previous patch.
Problem reported by Pádraig Brady <https://bugs.gnu.org/64058#11>.
* src/cksum.c (pclmul_supported): Also require AVX support
to use cksum_pclmul.
---
 src/cksum.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/cksum.c b/src/cksum.c
index 631ac3449..e935ba75c 100644
--- a/src/cksum.c
+++ b/src/cksum.c
@@ -160,7 +160,8 @@ static bool
 pclmul_supported (void)
 {
 # if USE_PCLMUL_CRC32
-  bool pclmul_enabled = 0 < __builtin_cpu_supports ("pclmul");
+  bool pclmul_enabled = (0 < __builtin_cpu_supports ("pclmul")
+                         && 0 < __builtin_cpu_supports ("avx"));
 
   if (cksum_debug)
     error (0, 0, "%s",
-- 
2.40.1

From 4ac941565fc1f7c1eb7954302f2ec20435fdf34c Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 14 Jun 2023 14:18:42 -0700
Subject: [PATCH 2/3] =?UTF-8?q?cksum,wc:=20don=E2=80=99t=20include=20<cpui?=
 =?UTF-8?q?d.h>?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/cksum.c [!CRCTAB && USE_PCLMUL_CRC32]:
* src/wc.c [USE_AVX2_WC_LINECOUNT]:
Don’t include <cpuid.h>; no longer needed.
---
 src/cksum.c | 4 +---
 src/wc.c    | 3 ---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/cksum.c b/src/cksum.c
index e935ba75c..352a0ba3a 100644
--- a/src/cksum.c
+++ b/src/cksum.c
@@ -141,9 +141,7 @@ main (void)
 # include "error.h"
 
 # include "cksum.h"
-# if USE_PCLMUL_CRC32
-#  include "cpuid.h"
-# else
+# if !USE_PCLMUL_CRC32
 #  define cksum_pclmul cksum_slice8
 # endif /* USE_PCLMUL_CRC32 */
 
diff --git a/src/wc.c b/src/wc.c
index 3708d0b8f..ebe83af4d 100644
--- a/src/wc.c
+++ b/src/wc.c
@@ -38,9 +38,6 @@
 #include "safe-read.h"
 #include "stat-size.h"
 #include "xbinary-io.h"
-#ifdef USE_AVX2_WC_LINECOUNT
-# include <cpuid.h>
-#endif
 
 #if !defined iswspace && !HAVE_ISWSPACE
 # define iswspace(wc) \
-- 
2.40.1

From f780a85985f5b069ba8597aaeac49eb74864926a Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 14 Jun 2023 14:52:37 -0700
Subject: [PATCH 3/3] cksum,wc: clean up hw capability checking
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/cksum.c (cksum_pclmul) [!CRCTAB && !USE_PCLMUL_CRC32]:
Remove macro.
(cksum_fp): No longer file-scope.
(pclmul_supported): Define only if USE_PCLMUL_CRC32.
This omits the debug output "using generic hardware support"
for simplicity and consistency with wc’s output.
(crc_sum_stream) [!USE_PCLMUL_32]: No need for static function pointer.
* src/wc.c (wc_lines_p) [USE_AVX2_WC_LINECOUNT]: No longer file-scope.
(wc) [USE_AVX2_WC_LINECOUNT]: Check for avx2 support at most once,
which was surely the code’s original intent.
(wc) [!USE_AVX2_WC_LINECOUNT]: No need for static function pointer.
---
 src/cksum.c | 29 ++++++++---------------------
 src/wc.c    | 14 ++++++--------
 2 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/src/cksum.c b/src/cksum.c
index 352a0ba3a..26bb29bdb 100644
--- a/src/cksum.c
+++ b/src/cksum.c
@@ -141,23 +141,14 @@ main (void)
 # include "error.h"
 
 # include "cksum.h"
-# if !USE_PCLMUL_CRC32
-#  define cksum_pclmul cksum_slice8
-# endif /* USE_PCLMUL_CRC32 */
 
 /* Number of bytes to read at once.  */
 # define BUFLEN (1 << 16)
 
-
-static bool
-cksum_slice8 (FILE *fp, uint_fast32_t *crc_out, uintmax_t *length_out);
-static bool
-  (*cksum_fp)(FILE *, uint_fast32_t *, uintmax_t *);
-
+# if USE_PCLMUL_CRC32
 static bool
 pclmul_supported (void)
 {
-# if USE_PCLMUL_CRC32
   bool pclmul_enabled = (0 < __builtin_cpu_supports ("pclmul")
                          && 0 < __builtin_cpu_supports ("avx"));
 
@@ -168,12 +159,8 @@ pclmul_supported (void)
             : _("pclmul support not detected")));
 
   return pclmul_enabled;
-# else
-  if (cksum_debug)
-    error (0, 0, "%s", _("using generic hardware support"));
-  return false;
-# endif /* USE_PCLMUL_CRC32 */
 }
+# endif /* USE_PCLMUL_CRC32 */
 
 static bool
 cksum_slice8 (FILE *fp, uint_fast32_t *crc_out, uintmax_t *length_out)
@@ -238,13 +225,13 @@ crc_sum_stream (FILE *stream, void *resstream, uintmax_t *length)
   uintmax_t total_bytes = 0;
   uint_fast32_t crc = 0;
 
+# if USE_PCLMUL_CRC32
+  static bool (*cksum_fp) (FILE *, uint_fast32_t *, uintmax_t *);
   if (! cksum_fp)
-    {
-       if (pclmul_supported ())
-         cksum_fp = cksum_pclmul;
-       else
-         cksum_fp = cksum_slice8;
-    }
+    cksum_fp = pclmul_supported () ? cksum_pclmul : cksum_slice8;
+#else
+  bool (*cksum_fp) (FILE *, uint_fast32_t *, uintmax_t *) = cksum_slice8;
+#endif
 
   if (! cksum_fp (stream, &crc, &total_bytes))
     return -1;
diff --git a/src/wc.c b/src/wc.c
index ebe83af4d..30214655c 100644
--- a/src/wc.c
+++ b/src/wc.c
@@ -54,18 +54,12 @@
 /* Size of atomic reads. */
 #define BUFFER_SIZE (16 * 1024)
 
-static bool
-wc_lines (char const *file, int fd, uintmax_t *lines_out,
-          uintmax_t *bytes_out);
 #ifdef USE_AVX2_WC_LINECOUNT
 /* From wc_avx2.c */
 extern bool
 wc_lines_avx2 (char const *file, int fd, uintmax_t *lines_out,
                uintmax_t *bytes_out);
 #endif
-static bool
-(*wc_lines_p) (char const *file, int fd, uintmax_t *lines_out,
-                uintmax_t *bytes_out) = wc_lines;
 
 static bool debug;
 
@@ -441,8 +435,12 @@ wc (int fd, char const *file_x, struct fstatus *fstatus, off_t current_pos)
   else if (!count_chars && !count_complicated)
     {
 #ifdef USE_AVX2_WC_LINECOUNT
-      if (avx2_supported ())
-        wc_lines_p = wc_lines_avx2;
+      static bool (*wc_lines_p) (char const *, int, uintmax_t *, uintmax_t *);
+      if (!wc_lines_p)
+        wc_lines_p = avx2_supported () ? wc_lines_avx2 : wc_lines;
+#else
+      bool (*wc_lines_p) (char const *, int, uintmax_t *, uintmax_t *)
+        = wc_lines;
 #endif
 
       /* Use a separate loop when counting only lines or lines and bytes --
-- 
2.40.1