[PATCH] xfs_scrub: fix strerror_r usage yet again

[PATCH] 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 glibc version can return any random
pointer and ignore the buffer, 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>
---
 scrub/common.c |   27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/scrub/common.c b/scrub/common.c
index 0a5970f9c066f7..030beadb1767fb 100644
--- a/scrub/common.c
+++ b/scrub/common.c
@@ -126,8 +126,31 @@ __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);
+		intptr_t res;
+
+		/*
+		 * GNU strerror_r returns a pointer to a string on success, but
+		 * the returned pointer might point to a static buffer and not
+		 * buf.  XSI strerror_r returns 0 on success, -1 on error.
+		 * Magic #defines influence which variant you might get.
+		 *
+		 * 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.
+		 *
+		 * To handle all this, we set buf to an empty string, and if
+		 * the function returns zero but buf now contains a non-empty
+		 * string, we assume the XSI version and print buf.  If the
+		 * function returns a positive value, we assume that's a
+		 * pointer to a string and print whatever the return value is.
+		 */
+		buf[0] = 0;
+		res = (intptr_t)strerror_r(error, buf, DESCR_BUFSZ);
+		if (res == 0 && strlen(buf) > 0)
+			fprintf(stream, _("%s."), buf);
+		else if (res > 0)
+			fprintf(stream, _("%s."), (char *)res);
 	} else {
 		va_start(args, format);
 		vfprintf(stream, format, args);

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

[Christoph Hellwig]

On Thu, Sep 18, 2025 at 12:48:36PM -0700, Darrick J. Wong wrote:
> "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.

That really makes things worse.  I think we'll just want ifdefs for the
two versions if there is no better option.

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

[Darrick J. Wong]

On Fri, Sep 19, 2025 at 07:47:30AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 18, 2025 at 12:48:36PM -0700, Darrick J. Wong wrote:
> > "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.
> 
> That really makes things worse.  I think we'll just want ifdefs for the
> two versions if there is no better option.

But what is there to #ifdef on?  _GNU_SOURCE is always included in
CFLAGS by builddefs.in even if we're not building against GNU libc
and doesn't define any new symbols so you can figure out which version
you got:

/* Reentrant version of `strerror'.
   There are 2 flavors of `strerror_r', GNU which returns the string
   and may or may not use the supplied temporary buffer and POSIX one
   which fills the string into the buffer.
   To use the POSIX version, -D_XOPEN_SOURCE=600 or -D_POSIX_C_SOURCE=200112L
   without -D_GNU_SOURCE is needed, otherwise the GNU version is
   preferred.  */
# if defined __USE_XOPEN2K && !defined __USE_GNU
/* Fill BUF with a string describing the meaning of the `errno' code in
   ERRNUM.  */
#  ifdef __REDIRECT_NTH
extern int __REDIRECT_NTH (strerror_r,
			   (int __errnum, char *__buf, size_t __buflen),
			   __xpg_strerror_r) __nonnull ((2))
    __attr_access ((__write_only__, 2, 3));
#  else
extern int __xpg_strerror_r (int __errnum, char *__buf, size_t __buflen)
     __THROW __nonnull ((2)) __attr_access ((__write_only__, 2, 3));
#   define strerror_r __xpg_strerror_r
#  endif
# else
/* If a temporary buffer is required, at most BUFLEN bytes of BUF will be
   used.  */
extern char *strerror_r (int __errnum, char *__buf, size_t __buflen)
     __THROW __nonnull ((2)) __wur  __attr_access ((__write_only__, 2, 3));
# endif

I tried turning off GNU_SOURCE but that broke the build even worse and I
don't feel like spending a whole lot of time fixing up all the damage
for the sake of a supporting an alt libc that I don't know how to build
against and don't much care about.

I suppose configure could detect which version you're getting, and set
its own #define that would enable us to #ifdef the callsite.  But this
"same symbol, different return types" is a whole mess of stupidity, to
say nothing of advisory feature enablement via preprocessor defines.

Anyway, v2 on its way.

--D