[PATCH v2] xfs_scrub: fix strerror_r usage yet again

[PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In commit 75faf2bc907584, someone tried to fix scrub to use the POSIX
version of strerror_r so that the build would work with musl.
Unfortunately, neither the author nor myself remembered that GNU libc
imposes its own version any time _GNU_SOURCE is defined, which
builddefs.in always does.  Regrettably, the POSIX and GNU versions have
different return types and the GNU version can return any random
pointer, so now this code is broken on glibc.

"Fix" this standards body own goal by casting the return value to
intptr_t and employing some gross heuristics to guess at the location of
the actual error string.

Fixes: 75faf2bc907584 ("xfs_scrub: Use POSIX-conformant strerror_r")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
v2: go the autoconf route
---
 configure.ac          |    1 +
 include/builddefs.in  |    1 +
 m4/package_libcdev.m4 |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 scrub/Makefile        |    4 ++++
 scrub/common.c        |    8 ++++++--
 5 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index d2407cb5de5af2..df19379b02ba55 100644
--- a/configure.ac
+++ b/configure.ac
@@ -183,6 +183,7 @@ AC_CONFIG_CROND_DIR
 AC_CONFIG_UDEV_RULE_DIR
 AC_HAVE_BLKID_TOPO
 AC_HAVE_TRIVIAL_AUTO_VAR_INIT
+AC_STRERROR_R_RETURNS_STRING
 
 if test "$enable_ubsan" = "yes" || test "$enable_ubsan" = "probe"; then
         AC_PACKAGE_CHECK_UBSAN
diff --git a/include/builddefs.in b/include/builddefs.in
index 93b5c75155c0f4..5aa5742bb31b9e 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -241,6 +241,7 @@ CROND_DIR = @crond_dir@
 HAVE_UDEV = @have_udev@
 UDEV_RULE_DIR = @udev_rule_dir@
 HAVE_LIBURCU_ATOMIC64 = @have_liburcu_atomic64@
+STRERROR_R_RETURNS_STRING = @strerror_r_returns_string@
 
 GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall -Werror -Wextra -Wno-unused-parameter
 #	   -Wbitwise -Wno-transparent-union -Wno-old-initializer -Wno-decl
diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
index ce1ba47264659c..c5538c30d2518a 100644
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -301,3 +301,49 @@ syscall(__NR_file_getattr, 0, 0, 0, 0, 0);
        AC_MSG_RESULT(no))
     AC_SUBST(have_file_getattr)
   ])
+
+#
+# Check if strerror_r returns an int, as opposed to a char *, because there are
+# two versions of this function, with differences that are hard to detect.
+#
+# GNU strerror_r returns a pointer to a string on success, but the returned
+# pointer might point to a static buffer and not buf, so you have to use the
+# return value.  The declaration has the __warn_unused_result__ attribute to
+# enforce this.
+#
+# XSI strerror_r always writes to buf and returns 0 on success, -1 on error.
+#
+# How do you select a particular version?  By defining macros, of course!
+# _GNU_SOURCE always gets you the GNU version, and _POSIX_C_SOURCE >= 200112L
+# gets you the XSI version but only if _GNU_SOURCE isn't defined.
+#
+# The build system #defines _GNU_SOURCE unconditionally, so when compiling
+# against glibc we get the GNU version.  However, when compiling against musl,
+# the _GNU_SOURCE definition does nothing and we get the XSI version anyway.
+# Not definining _GNU_SOURCE breaks the build in many areas, so we'll create
+# yet another #define for just this weird quirk so that we can patch around it
+# in the one place we need it.
+#
+# Note that we have to force erroring out on the int conversion warnings
+# because C doesn't consider it a hard error to cast a char pointer to an int
+# even when CFLAGS contains -std=gnu11.
+AC_DEFUN([AC_STRERROR_R_RETURNS_STRING],
+  [AC_MSG_CHECKING([if strerror_r returns char *])
+    OLD_CFLAGS="$CFLAGS"
+    CFLAGS="$CFLAGS -Wall -Werror"
+    AC_LINK_IFELSE(
+    [AC_LANG_PROGRAM([[
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <string.h>
+  ]], [[
+char buf[1024];
+puts(strerror_r(0, buf, sizeof(buf)));
+  ]])
+    ],
+       strerror_r_returns_string=yes
+       AC_MSG_RESULT(yes),
+       AC_MSG_RESULT(no))
+    CFLAGS="$OLD_CFLAGS"
+    AC_SUBST(strerror_r_returns_string)
+  ])
diff --git a/scrub/Makefile b/scrub/Makefile
index 3636a47942e98e..6375d77a291bcb 100644
--- a/scrub/Makefile
+++ b/scrub/Makefile
@@ -105,6 +105,10 @@ CFILES += unicrash.c
 LCFLAGS += -DHAVE_LIBICU $(LIBICU_CFLAGS)
 endif
 
+ifeq ($(STRERROR_R_RETURNS_STRING),yes)
+LCFLAGS += -DSTRERROR_R_RETURNS_STRING
+endif
+
 # Automatically trigger a media scan once per month
 XFS_SCRUB_ALL_AUTO_MEDIA_SCAN_INTERVAL=1mo
 
diff --git a/scrub/common.c b/scrub/common.c
index 9437d0abb8698b..9a33e2a9d54ed4 100644
--- a/scrub/common.c
+++ b/scrub/common.c
@@ -126,8 +126,12 @@ __str_out(
 	fprintf(stream, "%s%s: %s: ", stream_start(stream),
 			_(err_levels[level].string), descr);
 	if (error) {
-		strerror_r(error, buf, DESCR_BUFSZ);
-		fprintf(stream, _("%s."), buf);
+#ifdef STRERROR_R_RETURNS_STRING
+		fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ));
+#else
+		if (strerror_r(error, buf, DESCR_BUFSZ) == 0)
+			fprintf(stream, _("%s."), buf);
+#endif
 	} else {
 		va_start(args, format);
 		vfprintf(stream, format, args);

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[A. Wilcox]

> On Sep 19, 2025, at 11:14, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In commit 75faf2bc907584, someone tried to fix scrub to use the POSIX
> version of strerror_r so that the build would work with musl.
> Unfortunately, neither the author nor myself remembered that GNU libc
> imposes its own version any time _GNU_SOURCE is defined, which
> builddefs.in always does.  Regrettably, the POSIX and GNU versions have
> different return types and the GNU version can return any random
> pointer, so now this code is broken on glibc.
> 
> "Fix" this standards body own goal by casting the return value to
> intptr_t and employing some gross heuristics to guess at the location of
> the actual error string.
> 
> Fixes: 75faf2bc907584 ("xfs_scrub: Use POSIX-conformant strerror_r")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> v2: go the autoconf route
> ---
> configure.ac          |    1 +
> include/builddefs.in  |    1 +
> m4/package_libcdev.m4 |   46 ++++++++++++++++++++++++++++++++++++++++++++++
> scrub/Makefile        |    4 ++++
> scrub/common.c        |    8 ++++++--
> 5 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index d2407cb5de5af2..df19379b02ba55 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -183,6 +183,7 @@ AC_CONFIG_CROND_DIR
> AC_CONFIG_UDEV_RULE_DIR
> AC_HAVE_BLKID_TOPO
> AC_HAVE_TRIVIAL_AUTO_VAR_INIT
> +AC_STRERROR_R_RETURNS_STRING
> 
> if test "$enable_ubsan" = "yes" || test "$enable_ubsan" = "probe"; then
>         AC_PACKAGE_CHECK_UBSAN
> diff --git a/include/builddefs.in b/include/builddefs.in
> index 93b5c75155c0f4..5aa5742bb31b9e 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -241,6 +241,7 @@ CROND_DIR = @crond_dir@
> HAVE_UDEV = @have_udev@
> UDEV_RULE_DIR = @udev_rule_dir@
> HAVE_LIBURCU_ATOMIC64 = @have_liburcu_atomic64@
> +STRERROR_R_RETURNS_STRING = @strerror_r_returns_string@
> 
> GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall -Werror -Wextra -Wno-unused-parameter
> #   -Wbitwise -Wno-transparent-union -Wno-old-initializer -Wno-decl
> diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
> index ce1ba47264659c..c5538c30d2518a 100644
> --- a/m4/package_libcdev.m4
> +++ b/m4/package_libcdev.m4
> @@ -301,3 +301,49 @@ syscall(__NR_file_getattr, 0, 0, 0, 0, 0);
>        AC_MSG_RESULT(no))
>     AC_SUBST(have_file_getattr)
>   ])
> +
> +#
> +# Check if strerror_r returns an int, as opposed to a char *, because there are
> +# two versions of this function, with differences that are hard to detect.
> +#
> +# GNU strerror_r returns a pointer to a string on success, but the returned
> +# pointer might point to a static buffer and not buf, so you have to use the
> +# return value.  The declaration has the __warn_unused_result__ attribute to
> +# enforce this.
> +#
> +# XSI strerror_r always writes to buf and returns 0 on success, -1 on error.
> +#
> +# How do you select a particular version?  By defining macros, of course!
> +# _GNU_SOURCE always gets you the GNU version, and _POSIX_C_SOURCE >= 200112L
> +# gets you the XSI version but only if _GNU_SOURCE isn't defined.
> +#
> +# The build system #defines _GNU_SOURCE unconditionally, so when compiling
> +# against glibc we get the GNU version.  However, when compiling against musl,
> +# the _GNU_SOURCE definition does nothing and we get the XSI version anyway.
> +# Not definining _GNU_SOURCE breaks the build in many areas, so we'll create
> +# yet another #define for just this weird quirk so that we can patch around it
> +# in the one place we need it.
> +#
> +# Note that we have to force erroring out on the int conversion warnings
> +# because C doesn't consider it a hard error to cast a char pointer to an int
> +# even when CFLAGS contains -std=gnu11.
> +AC_DEFUN([AC_STRERROR_R_RETURNS_STRING],
> +  [AC_MSG_CHECKING([if strerror_r returns char *])
> +    OLD_CFLAGS="$CFLAGS"
> +    CFLAGS="$CFLAGS -Wall -Werror"
> +    AC_LINK_IFELSE(
> +    [AC_LANG_PROGRAM([[
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <string.h>
> +  ]], [[
> +char buf[1024];
> +puts(strerror_r(0, buf, sizeof(buf)));
> +  ]])
> +    ],
> +       strerror_r_returns_string=yes
> +       AC_MSG_RESULT(yes),
> +       AC_MSG_RESULT(no))
> +    CFLAGS="$OLD_CFLAGS"
> +    AC_SUBST(strerror_r_returns_string)
> +  ])
> diff --git a/scrub/Makefile b/scrub/Makefile
> index 3636a47942e98e..6375d77a291bcb 100644
> --- a/scrub/Makefile
> +++ b/scrub/Makefile
> @@ -105,6 +105,10 @@ CFILES += unicrash.c
> LCFLAGS += -DHAVE_LIBICU $(LIBICU_CFLAGS)
> endif
> 
> +ifeq ($(STRERROR_R_RETURNS_STRING),yes)
> +LCFLAGS += -DSTRERROR_R_RETURNS_STRING
> +endif
> +
> # Automatically trigger a media scan once per month
> XFS_SCRUB_ALL_AUTO_MEDIA_SCAN_INTERVAL=1mo
> 
> diff --git a/scrub/common.c b/scrub/common.c
> index 9437d0abb8698b..9a33e2a9d54ed4 100644
> --- a/scrub/common.c
> +++ b/scrub/common.c
> @@ -126,8 +126,12 @@ __str_out(
> fprintf(stream, "%s%s: %s: ", stream_start(stream),
> _(err_levels[level].string), descr);
> if (error) {
> - strerror_r(error, buf, DESCR_BUFSZ);
> - fprintf(stream, _("%s."), buf);
> +#ifdef STRERROR_R_RETURNS_STRING
> + fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ));
> +#else
> + if (strerror_r(error, buf, DESCR_BUFSZ) == 0)
> + fprintf(stream, _("%s."), buf);
> +#endif
> } else {
> va_start(args, format);
> vfprintf(stream, format, args);

I did check *build* on glibc, but I don’t think mine had warn_unused_result yet.
(It’s an older version.)

Thanks for fixing this, looks good to me.

The commit message isn’t accurate any more though.

Reviewed-by: A. Wilcox <AWilcox@Wilcox-Tech.com>

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Christoph Hellwig]

The autoconf maigc looks good (as good as autoconf can look anyway),
but why is this code using strerror_r to start with?  AFAIK on Linux
strerror is using thread local storage since the damn of time, so
just doing this as:

	fprintf(stream, _("%s."), strerror(error));

should be perfectly fine, while also simpler and more portable.

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Darrick J. Wong]

On Mon, Sep 22, 2025 at 10:01:17AM -0700, Christoph Hellwig wrote:
> The autoconf maigc looks good (as good as autoconf can look anyway),
> but why is this code using strerror_r to start with?  AFAIK on Linux
> strerror is using thread local storage since the damn of time, so
> just doing this as:
> 
> 	fprintf(stream, _("%s."), strerror(error));
> 
> should be perfectly fine, while also simpler and more portable.

But there's no guarantee that the implementation does this, is there?
The manpage doesn't say anything like that.

--D

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Christoph Hellwig]

On Tue, Sep 23, 2025 at 05:53:53PM -0700, Darrick J. Wong wrote:
> On Mon, Sep 22, 2025 at 10:01:17AM -0700, Christoph Hellwig wrote:
> > The autoconf maigc looks good (as good as autoconf can look anyway),
> > but why is this code using strerror_r to start with?  AFAIK on Linux
> > strerror is using thread local storage since the damn of time, so
> > just doing this as:
> > 
> > 	fprintf(stream, _("%s."), strerror(error));
> > 
> > should be perfectly fine, while also simpler and more portable.
> 
> But there's no guarantee that the implementation does this, is there?
> The manpage doesn't say anything like that.

To me this pretty clear reads that the return string is stable until
the next call to strerror as long as you only use it in the calling
thread:

"The strerror() function returns a pointer to a string that describes
the error code passed in the argument errnum,  ...
This string must not be modified by the application, and the returned
pointer will be invalidated on a subsequent call to strerror() or
strerror_l(), or if the thread that obtained the string exits.  No
other library function, including perror(3), will modify this string."

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Darrick J. Wong]

On Thu, Sep 25, 2025 at 12:23:48AM -0700, Christoph Hellwig wrote:
> On Tue, Sep 23, 2025 at 05:53:53PM -0700, Darrick J. Wong wrote:
> > On Mon, Sep 22, 2025 at 10:01:17AM -0700, Christoph Hellwig wrote:
> > > The autoconf maigc looks good (as good as autoconf can look anyway),
> > > but why is this code using strerror_r to start with?  AFAIK on Linux
> > > strerror is using thread local storage since the damn of time, so
> > > just doing this as:
> > > 
> > > 	fprintf(stream, _("%s."), strerror(error));
> > > 
> > > should be perfectly fine, while also simpler and more portable.
> > 
> > But there's no guarantee that the implementation does this, is there?
> > The manpage doesn't say anything like that.
> 
> To me this pretty clear reads that the return string is stable until
> the next call to strerror as long as you only use it in the calling
> thread:
> 
> "The strerror() function returns a pointer to a string that describes
> the error code passed in the argument errnum,  ...
> This string must not be modified by the application, and the returned
> pointer will be invalidated on a subsequent call to strerror() or
> strerror_l(), or if the thread that obtained the string exits.  No
> other library function, including perror(3), will modify this string."

Huh, I don't see "...or if the thread that obtained the string exits" in
my version of the strerror(3) manpage on Debian 12.  For that matter, it
still lists strerror() as MT-Unsafe.

Looking through the manpages git repo, I see that strerror actually does
get changed to MT-Safe in commit 01576f703f820e54602:

    strerror.3: Change strerror() reference from MT-Unsafe to MT-Safe

    The information in this patch was obtained from a glibc upstream patch,
    commit 28aff047818eb1726394296d27b9c7885340bead ("string: Implement
    strerror in terms of strerror_l").

    According to the patch above, for glibc versions >=2.32, strerror() is
    MT-Safe.

    Signed-off-by: Shani Leviim <sleviim@redhat.com>

Then looking through the glibc repository, that 28aff047818e commit
comes from:

https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=28aff047818eb1726394296d27b9c7885340bead;hp=9deec7c8bab24659e78172dd850f4ca37c57940c

which turns strerror into strerror_l.  This commit only appears in glibc
2.32, which appeared in August 2020.  That version first appears in
Debian 12 and RHEL 9, which are still fairly new.  musl apparently has
had strerror call strerror_l since at least 2011.

Has strerror() been designated as thread-safe at a POSIX level, or is
this just an implementation quirk of these two C libraries?  strerror
definitely wasn't thread-safe on glibc when I wrote this program.

--D

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Holger Hoffstätte]

On 2025-09-25 22:04, Darrick J. Wong wrote:
> Has strerror() been designated as thread-safe at a POSIX level, or is
> this just an implementation quirk of these two C libraries?  strerror
> definitely wasn't thread-safe on glibc when I wrote this program.

It still is not:
https://pubs.opengroup.org/onlinepubs/9799919799/functions/strerror.html

Pretty safe to say that this particular train has sailed.

cheers
Holger

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Darrick J. Wong]

On Thu, Sep 25, 2025 at 10:58:38PM +0200, Holger Hoffstätte wrote:
> On 2025-09-25 22:04, Darrick J. Wong wrote:
> > Has strerror() been designated as thread-safe at a POSIX level, or is
> > this just an implementation quirk of these two C libraries?  strerror
> > definitely wasn't thread-safe on glibc when I wrote this program.
> 
> It still is not:
> https://pubs.opengroup.org/onlinepubs/9799919799/functions/strerror.html
> 
> Pretty safe to say that this particular train has sailed.

Sailed off the end of the pier, yeah. ;)

Andrey: could you pick this one up, please?

--D

> cheers
> Holger
> 

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Christoph Hellwig]

On Thu, Sep 25, 2025 at 01:04:06PM -0700, Darrick J. Wong wrote:
> Then looking through the glibc repository, that 28aff047818e commit
> comes from:
> 
> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=28aff047818eb1726394296d27b9c7885340bead;hp=9deec7c8bab24659e78172dd850f4ca37c57940c
> 
> which turns strerror into strerror_l.  This commit only appears in glibc
> 2.32, which appeared in August 2020.  That version first appears in
> Debian 12 and RHEL 9, which are still fairly new.  musl apparently has
> had strerror call strerror_l since at least 2011.
> 
> Has strerror() been designated as thread-safe at a POSIX level, or is
> this just an implementation quirk of these two C libraries?  strerror
> definitely wasn't thread-safe on glibc when I wrote this program.

No POSIX gurantee (and I don't think that matters for xfs_scrub),
but glibc before wasn't exactly thread unsafe before.  It just simply
leaked the error buffer when it had to print "unknown erorr.." for
errors now known to it.

Re: [PATCH v2] xfs_scrub: fix strerror_r usage yet again

[Andrey Albershteyn]

On 2025-09-26 09:27:16, Darrick J. Wong wrote:
> On Thu, Sep 25, 2025 at 10:58:38PM +0200, Holger Hoffstätte wrote:
> > On 2025-09-25 22:04, Darrick J. Wong wrote:
> > > Has strerror() been designated as thread-safe at a POSIX level, or is
> > > this just an implementation quirk of these two C libraries?  strerror
> > > definitely wasn't thread-safe on glibc when I wrote this program.
> > 
> > It still is not:
> > https://pubs.opengroup.org/onlinepubs/9799919799/functions/strerror.html
> > 
> > Pretty safe to say that this particular train has sailed.
> 
> Sailed off the end of the pier, yeah. ;)
> 
> Andrey: could you pick this one up, please?

I will include this one in next for-next

-- 
- Andrey