Re: Behaviour of strverscmp(3)

Re: Behaviour of strverscmp(3)

[Simon Josefsson via Gnulib discussion list]

[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]

Thanks for report, Dmitry.  I am slowly coming back to this.  I have
noticed that Cygwin (via MSYS2) has the same strverscmp as musl:

https://cygwin.com/cgit/newlib-cygwin/tree/newlib/libc/string/strverscmp.c

Compare against musl strverscmp:

https://git.musl-libc.org/cgit/musl/tree/src/string/strverscmp.c

Since gsasl (and many other projects) gets strverscmp() from gnulib, I'm
cc'ing the bug-gnulib list.  I think gnulib should detect and work
around this buggy strverscmp.  The documentation says the function is
missing on all non-glibc platforms, but this is not the case, see:

https://www.gnu.org/software/gnulib/manual/html_node/strverscmp.html

I don't have time to work on a patch for gnulib now, but this e-mail
will serve as a reminder... but happy if someone else has ideas on how
to resolve it in gnulib.  See reproducer below; I recall seeing other
problems too such as strverscmp("1.7", "1.7") behaving different.

Compare to the glibc/gnulib implementation:

https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/strverscmp.c

/Simon

Dmitry Bogatov <oht-tfnfy#tah.bet#i1@kaction.cc> writes:

> Hello.
>
> While trying to building gsasl statically with musl library as part of
> Nixpkgs distribution, I noticed that test built from tests/version.c
> fails when built with musl library. After a bit of troubleshooting, I
> can pinpoint the reason -- different behaviour of "strverscmp" from
> glibc and musl.
>
> Example code:
>
> #include <string.h>
> #include <stdio.h>
>
> int main()
> {
> 	int value = strverscmp("UNKNOWN", "2.2.0");
> 	printf("%d\n", value);
> 	return 0;
> }
>
> Under glibc value "35" is printed (positive), under musl value "-1" is
> printed (negative). Not sure what is the correct solution for the
> issue, so I cross-post into two lists.
>
> For now I plan to patch-out this particular test. Thank you.
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 255 bytes --]

Re: Behaviour of strverscmp(3)

[Bruno Haible]

Dmitry Bogatov wrote:
> > Example code:
> >
> > #include <string.h>
> > #include <stdio.h>
> >
> > int main()
> > {
> > 	int value = strverscmp("UNKNOWN", "2.2.0");
> > 	printf("%d\n", value);
> > 	return 0;
> > }
> >
> > Under glibc value "35" is printed (positive), under musl value "-1" is
> > printed (negative).

Indeed, I can reproduce this with
  - musl libc 1.2.3 and older (Alpine Linux 3.17 and older),
  - Cygwin 2.9.0.

> > Not sure what is the correct solution for the
> > issue, so I cross-post into two lists.

Since the issue has meanwhile been fixed in musl libc 1.2.4
https://git.musl-libc.org/cgit/musl/commit/src/string/strverscmp.c?id=b50eb8c36c20f967bd0ed70c0b0db38a450886ba
the glibc behaviour is the correct one.

I'm applying the patch below.

> > While trying to building gsasl statically with musl library as part of
> > Nixpkgs distribution

Nixpkg must be using an old musl libc, then. The newest one is 1.2.4.

Simon Josefsson wrote:
> https://cygwin.com/cgit/newlib-cygwin/tree/newlib/libc/string/strverscmp.c

So, I've forwarded the report to the Cygwin people:
https://sourceware.org/pipermail/cygwin/2024-January/255085.html

> Cygwin (via MSYS2)

Errr. Cygwin does not use MSYS2. MSYS2 cannibalizes the Cygwin sources, not
the other way around.


2024-01-02  Bruno Haible  <bruno@clisp.org>

	strverscmp: Work around bug in musl libc 1.2.3 and in Cygwin.
	Reported by Dmitry Bogatov <KAction@gnu.org> via Simon Josefsson in
	<https://lists.gnu.org/archive/html/bug-gnulib/2024-01/msg00002.html>.
	* m4/string_h.m4 (gl_STRING_H_DEFAULTS): Initialize REPLACE_STRVERSCMP.
	* m4/strverscmp.m4 (gl_FUNC_STRVERSCMP): Test whether strverscmp works
	and set REPLACE_STRVERSCMP if not.
	* lib/string.in.h (strverscmp): Consider REPLACE_STRVERSCMP.
	* modules/strverscmp (Depends-on, configure.ac): Likewise.
	* modules/string (Makefile.am): Substitute REPLACE_STRVERSCMP.
	* tests/test-strverscmp.c (main): Add test cases suggested by Dmitry
	Bogatov and by Simon Josefsson.
	* doc/glibc-functions/strverscmp.texi: Mention the musl and Cygwin bug.
	Update version info regarding FreeBSD.

diff --git a/doc/glibc-functions/strverscmp.texi b/doc/glibc-functions/strverscmp.texi
index 5882d223e1..9e37143fa4 100644
--- a/doc/glibc-functions/strverscmp.texi
+++ b/doc/glibc-functions/strverscmp.texi
@@ -20,8 +20,13 @@
 Portability problems fixed by Gnulib:
 @itemize
 @item
-This function is missing on all non-glibc platforms:
-macOS 11.1, FreeBSD 13.0, NetBSD 9.0, OpenBSD 6.7, Minix 3.1.8, AIX 5.1, HP-UX 11, IRIX 6.5, Solaris 11.4, Cygwin 1.7.x, mingw, MSVC 14, Android 9.0.
+This function is missing on many platforms:
+macOS 11.1, FreeBSD 13.1, NetBSD 9.0, OpenBSD 6.7, Minix 3.1.8, AIX 5.1, HP-UX 11, IRIX 6.5, Solaris 11.4, Cygwin 1.7.x, mingw, MSVC 14, Android 9.0.
+@item
+This function treats ASCII letters as smaller than a digit sequence
+on some platforms:
+@c https://git.musl-libc.org/cgit/musl/commit/src/string/strverscmp.c?id=b50eb8c36c20f967bd0ed70c0b0db38a450886ba
+musl libc 1.2.3, Cygwin 3.4.6.
 @end itemize
 
 Portability problems not fixed by Gnulib:
diff --git a/lib/string.in.h b/lib/string.in.h
index 66be871a57..01ea3e3913 100644
--- a/lib/string.in.h
+++ b/lib/string.in.h
@@ -1419,12 +1419,22 @@ _GL_WARN_ON_USE (strsignal, "strsignal is unportable - "
 #endif
 
 #if @GNULIB_STRVERSCMP@
-# if !@HAVE_STRVERSCMP@
+# if @REPLACE_STRVERSCMP@
+#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+#   define strverscmp rpl_strverscmp
+#  endif
+_GL_FUNCDECL_RPL (strverscmp, int, (const char *, const char *)
+                                   _GL_ATTRIBUTE_PURE
+                                   _GL_ARG_NONNULL ((1, 2)));
+_GL_CXXALIAS_RPL (strverscmp, int, (const char *, const char *));
+# else
+#  if !@HAVE_STRVERSCMP@
 _GL_FUNCDECL_SYS (strverscmp, int, (const char *, const char *)
                                    _GL_ATTRIBUTE_PURE
                                    _GL_ARG_NONNULL ((1, 2)));
-# endif
+#  endif
 _GL_CXXALIAS_SYS (strverscmp, int, (const char *, const char *));
+# endif
 _GL_CXXALIASWARN (strverscmp);
 #elif defined GNULIB_POSIXCHECK
 # undef strverscmp
diff --git a/m4/string_h.m4 b/m4/string_h.m4
index 3cbcbc7487..8b12101447 100644
--- a/m4/string_h.m4
+++ b/m4/string_h.m4
@@ -5,7 +5,7 @@
 # gives unlimited permission to copy and/or distribute it,
 # with or without modifications, as long as this notice is preserved.
 
-# serial 37
+# serial 38
 
 # Written by Paul Eggert.
 
@@ -146,5 +146,6 @@ AC_DEFUN([gl_STRING_H_DEFAULTS]
   REPLACE_STRERROR_R=0;         AC_SUBST([REPLACE_STRERROR_R])
   REPLACE_STRERRORNAME_NP=0;    AC_SUBST([REPLACE_STRERRORNAME_NP])
   REPLACE_STRSIGNAL=0;          AC_SUBST([REPLACE_STRSIGNAL])
+  REPLACE_STRVERSCMP=0;         AC_SUBST([REPLACE_STRVERSCMP])
   UNDEFINE_STRTOK_R=0;          AC_SUBST([UNDEFINE_STRTOK_R])
 ])
diff --git a/m4/strverscmp.m4 b/m4/strverscmp.m4
index a0eef7bd92..748272abec 100644
--- a/m4/strverscmp.m4
+++ b/m4/strverscmp.m4
@@ -1,4 +1,4 @@
-# strverscmp.m4 serial 9
+# strverscmp.m4 serial 10
 dnl Copyright (C) 2002, 2005-2024 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -13,6 +13,37 @@ AC_DEFUN([gl_FUNC_STRVERSCMP]
   AC_CHECK_FUNCS([strverscmp])
   if test $ac_cv_func_strverscmp = no; then
     HAVE_STRVERSCMP=0
+  else
+    AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
+    AC_CACHE_CHECK([whether strverscmp works],
+      [gl_cv_func_strverscmp_works],
+      [dnl Detect musl-1.2.3 and Cygwin 3.4.6 bug.
+       AC_RUN_IFELSE(
+         [AC_LANG_SOURCE([[
+#include <string.h>
+int main ()
+{
+  return strverscmp ("UNKNOWN", "2.2.0") <= 0;
+}
+         ]])],
+         [gl_cv_func_strverscmp_works=yes],
+         [gl_cv_func_strverscmp_works=no],
+         [case "$host_os" in
+                                # Guess yes on glibc systems.
+            *-gnu* | gnu*)      gl_cv_func_strverscmp_works="guessing yes" ;;
+                                # Guess no on musl systems.
+            *-musl* | midipix*) gl_cv_func_strverscmp_works="guessing no" ;;
+                                # Guess no on Cygwin.
+            cygwin*)            gl_cv_func_strverscmp_works="guessing no" ;;
+                                # If we don't know, obey --enable-cross-guesses.
+            *)                  gl_cv_func_strverscmp_works="$gl_cross_guess_normal" ;;
+          esac
+         ])
+      ])
+    case "$gl_cv_func_strverscmp_works" in
+      *yes) ;;
+      *) REPLACE_STRVERSCMP=1 ;;
+    esac
   fi
 ])
 
diff --git a/modules/string b/modules/string
index 8fbcc542c5..acbd614dcd 100644
--- a/modules/string
+++ b/modules/string
@@ -125,6 +125,7 @@ string.h: string.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNULL_H
 	      -e 's|@''REPLACE_STRERROR_R''@|$(REPLACE_STRERROR_R)|g' \
 	      -e 's|@''REPLACE_STRERRORNAME_NP''@|$(REPLACE_STRERRORNAME_NP)|g' \
 	      -e 's|@''REPLACE_STRSIGNAL''@|$(REPLACE_STRSIGNAL)|g' \
+	      -e 's|@''REPLACE_STRVERSCMP''@|$(REPLACE_STRVERSCMP)|g' \
 	      -e 's|@''UNDEFINE_STRTOK_R''@|$(UNDEFINE_STRTOK_R)|g' \
 	      -e '/definitions of _GL_FUNCDECL_RPL/r $(CXXDEFS_H)' \
 	      -e '/definition of _GL_ARG_NONNULL/r $(ARG_NONNULL_H)' \
diff --git a/modules/strverscmp b/modules/strverscmp
index a80d1dd1d7..f4f76c7d0c 100644
--- a/modules/strverscmp
+++ b/modules/strverscmp
@@ -7,13 +7,14 @@ m4/strverscmp.m4
 
 Depends-on:
 extensions
-libc-config  [test $HAVE_STRVERSCMP = 0]
-stdint       [test $HAVE_STRVERSCMP = 0]
+libc-config  [test $HAVE_STRVERSCMP = 0 || test $REPLACE_STRVERSCMP = 1]
+stdint       [test $HAVE_STRVERSCMP = 0 || test $REPLACE_STRVERSCMP = 1]
 string
 
 configure.ac:
 gl_FUNC_STRVERSCMP
-gl_CONDITIONAL([GL_COND_OBJ_STRVERSCMP], [test $HAVE_STRVERSCMP = 0])
+gl_CONDITIONAL([GL_COND_OBJ_STRVERSCMP],
+               [test $HAVE_STRVERSCMP = 0 || test $REPLACE_STRVERSCMP = 1])
 AM_COND_IF([GL_COND_OBJ_STRVERSCMP], [
   gl_PREREQ_STRVERSCMP
 ])
diff --git a/tests/test-strverscmp.c b/tests/test-strverscmp.c
index a7fbcdb993..2706572bc6 100644
--- a/tests/test-strverscmp.c
+++ b/tests/test-strverscmp.c
@@ -30,6 +30,7 @@ main (void)
 {
   ASSERT (strverscmp ("", "") == 0);
   ASSERT (strverscmp ("a", "a") == 0);
+  ASSERT (strverscmp ("1.7", "1.7") == 0);
   ASSERT (strverscmp ("a", "b") < 0);
   ASSERT (strverscmp ("b", "a") > 0);
   ASSERT (strverscmp ("000", "00") < 0);
@@ -55,5 +56,13 @@ main (void)
     ASSERT (strverscmp (c, a) > 0);
   }
 
+  /* From Dmitry Bogatov.  */
+  {
+    static char const a[] = "UNKNOWN";
+    static char const b[] = "2.2.0";
+    ASSERT (strverscmp (a, b) > 0);
+    ASSERT (strverscmp (b, a) < 0);
+  }
+
   return 0;
 }