Core:test_path_length:3 test failure when linked against musl

[Marecki]

Since at least version 4.8.29, Midnight Commander fails one of its tests when linked against MUSL:

path_len.c:116:F:Core:test_path_length:3: Assertion 'actual_length == data->expected_length' failed: actual_length == 12, data->expected_length == 38

The same test passes when mc is linked against glibc. I suspect this might have something to do with how limited iconv support is in MUSL but unfortunately I know nowhere nearly enough about either that libc or mc internals to attempt to fix the problem myself.

Version information for both glibc- and musl-linked mc-4.8.30. In either case Midnight Commander was installed directly into a clean test system so different libc implementation should have been the only difference between the two:

* built against musl *

GNU Midnight Commander 4.8.30
Built with GLib 2.76.3
Built with S-Lang 2.3.2 with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm
With multiple codepages support
With ext2fs attributes support
Virtual File Systems:

cpiofs, tarfs, sfs, extfs, ext2undelfs, ftpfs, fish

Data types:

char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

* built agains glibc *

GNU Midnight Commander 4.8.30
Built with GLib 2.76.3
Built with S-Lang 2.3.2 with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm
With internationalization support
With multiple codepages support
With ext2fs attributes support
Virtual File Systems:

cpiofs, tarfs, sfs, extfs, ext2undelfs, ftpfs, fish

Data types:

char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

[zaytsev]

cfarm27 is running x86 VM (32-bit) with musl libc on Alpine 3.19.3 - probably the current situation can be checked there.

There is an unofficial musl mirror on GitHub:

​https://github.com/bminor/musl/blob/master/src/locale/codepages.h
​https://github.com/bminor/musl/blob/master/src/locale/iconv.c

It seems to support KOI8-R used in tests. Obviously, the conversion from UTF-8 to KOI8-R is completely wrong, because it should take at more bytes to represent...

Checked the name mapping functions and it seems any name should be accepted (lower/upper case, with or without dashes).

Strange :(

[zaytsev]

P.S. Alpine developer complained in the ticket about Illumos: #3972 - not sure if related.

[zaytsev]

I've installed alpine and path_len test is fixed by the following patch like on Illumos:

diff --git a/tests/lib/vfs/path_len.c b/tests/lib/vfs/path_len.c
index 6bab6f551..24beca8ac 100644
--- a/tests/lib/vfs/path_len.c
+++ b/tests/lib/vfs/path_len.c
@@ -42,7 +42,7 @@
 static void
 setup (void)
 {
-    str_init_strings (NULL);
+    str_init_strings ("UTF-8");
 
     vfs_init ();
     vfs_init_localfs ();

Unfortunately, there is one more test that fails: vfs_path_string_convert. Parts 4,5,6,7,8 are finishing with SIGABRT or SIGSEGV. Sadly it's not as easy as forcing UTF-8.

One part of it is that the test uses IBM866, and Alpine MUSL only has CP866. But replacing IBM866 with CP-866 only fixes the segfaults, but then the tests keep failing with SIGABRT.

Somehow it seems that the core of the problem is the same as on Illumos. We apparently handle invalid conversions incorrectly on non-GNU iconvs (or maybe it's glib?). Luckily all the other test pass and --disable-charset also works.

Otherwise there is a compiler warning due to a redundant declaration and mc is spamming shell history and persistent history doesn't work with ash.

[zaytsev]

The environ warning on Alpine can be fixed with the patch from Tor:

​https://gitlab.torproject.org/tpo/core/tor/-/commit/81fe3e438b39cd14986247581d03cb0d0d650f1d

From bc66878bdea0250991fc99b2d023146f67a6f4bb Mon Sep 17 00:00:00 2001
From: Sebastian Hahn <sebastian@torproject.org>
Date: Sun, 19 Feb 2012 16:09:08 +0100
Subject: [PATCH] Don't redeclare environ if std headers already did

This would cause a redundant redeclaration warning on some versions of
Linux otherwise.
---
 configure.in        | 16 ++++++++++++++++
 src/common/compat.c | 10 ++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/configure.in b/configure.in
index 7b6d8fb023..7415ce8312 100644
--- a/configure.in
+++ b/configure.in
@@ -1086,6 +1086,17 @@ int main(int c, char **v) { puts(__FUNCTION__); }])],
   tor_cv_have_FUNCTION_macro=yes,
   tor_cv_have_FUNCTION_macro=no))
 
+AC_CACHE_CHECK([whether we have extern char **environ already declared],
+  tor_cv_have_environ_declared,
+  AC_COMPILE_IFELSE([AC_LANG_SOURCE([
+/* We define _GNU_SOURCE here because it is also defined in compat.c.
+ * Without it environ doesn't get declared. */
+#define _GNU_SOURCE
+#include <unistd.h>
+int main(int c, char **v) { char **t = environ; }])],
+  tor_cv_have_environ_declared=yes,
+  tor_cv_have_environ_declared=no))
+
 if test "$tor_cv_have_func_macro" = 'yes'; then
   AC_DEFINE(HAVE_MACRO__func__, 1, [Defined if the compiler supports __func__])
 fi
@@ -1099,6 +1110,11 @@ if test "$tor_cv_have_FUNCTION_macro" = 'yes'; then
            [Defined if the compiler supports __FUNCTION__])
 fi
 
+if test "$tor_cv_have_environ_declared" = 'yes'; then
+  AC_DEFINE(HAVE_EXTERN_ENVIRON_DECLARED__, 1,
+           [Defined if we have extern char **environ already declared])
+fi
+
 # $prefix stores the value of the --prefix command line option, or
 # NONE if the option wasn't set.  In the case that it wasn't set, make
 # it be the default, so that we can use it to expand directories now.
diff --git a/src/common/compat.c b/src/common/compat.c
index f25a8ac3b0..30bde3d1ca 100644
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -16,7 +16,11 @@
  * We also need it to make memmem get defined (where available)
  */
 /* XXXX023 We should just use AC_USE_SYSTEM_EXTENSIONS in our autoconf,
- * and get this (and other important stuff!) automatically */
+ * and get this (and other important stuff!) automatically. Once we do that,
+ * make sure to also change the extern char **environ detection in
+ * configure.in, because whether that is declared or not depends on whether
+ * we have _GNU_SOURCE defined! Maybe that means that once we take this out,
+ * we can also take out the configure check. */
 #define _GNU_SOURCE
 
 #include "compat.h"
@@ -1663,9 +1667,11 @@ make_path_absolute(char *fname)
 }
 
 #ifndef HAVE__NSGETENVIRON
-/* FreeBSD needs this; it doesn't seem to hurt other platforms. */
+#ifndef HAVE_EXTERN_ENVIRON_DECLARED__
+/* Some platforms declare environ under some circumstances, others don't. */
 extern char **environ;
 #endif
+#endif
 
 /** Return the current environment. This is a portable replacement for
  * 'environ'. */
-- 
GitLab

[andrew_b]

We can use Gnulib: ​https://git.savannah.gnu.org/cgit/gnulib.git/tree/m4/environ.m4

[zaytsev]

I wonder how much is left to fix on musl after Illumos fixes...

[zaytsev]

No way, all tests pass!

The warning, of course, is still there, but it can be fixed.

There is also a problem that mc spams shell history (my understanding is that alpine default is dash) with

  cd "`printf %b` `...`"

Not sure if there is a ticket for that. Maybe #3010 ?

[zaytsev]

The printf stuff is because of #2104.

[zaytsev]

I tried to import environ.m4 from gnulib, but it depends on other modules and has a whole crazy maintenance system, which we apparently don't use. It's not as simple as just downloading the file.

How did you do it before? Just copied the files and hoped that they work, or changed them to work? Or you are indeed using gnulib-tool?

[zaytsev]

Branch: 4495_alpine_support
Initial changeset:b8e2a376e32991f433337ca777444ab73fee0e6e

Apparently you copied gnulib stuff as needed and adjusted the dependencies. I tried to do this with environ.m4, but it is just too crazy. It needs module after module after module.

Either we re-integrate gnulib properly, or I suggest a simple solution with `AC_CHECK_DECLS'. It is imperfect, but I think it is no worse than our defines at the moment.

[andrew_b]

I've added a small fix.

[zaytsev]

I think that your fix is not correct. After that I get on macOS:

../../../lib/widget/input_complete.c:356:17: error: use of undeclared identifier 'environ'
  356 |         env_p = environ;
      |                 ^
1 error generated.

The reason is as follows:

Unlike the other ‘AC_CHECK_*S’ macros, when a symbol is not declared, HAVE_DECL_symbol is defined to ‘0’ instead of leaving HAVE_DECL_symbol undeclared. When you are sure that the check was performed, use HAVE_DECL_symbol in #if:

#if !HAVE_DECL_SYMBOL
extern char *symbol;
#endif

So negative HAVE_DECL checks should be always done with negation instead of ifndef, or the result will be always false.

[andrew_b]

Got it. Ignore my commit.

[zaytsev]

Merged to master as changeset:6fb0bb589ff072817f6ab7ffb206c185fa244a75.

git log --oneline dd12be7..6fb0bb5