Ticket #4642: extract and re-implement str_chomp to fix buffer overflow, add tests

Found in Alpine/musl on s390x, confirmed on aarch64 using valgrind - introduced
in 65a7278d8a34abe804299d721749bc747e4a4833:

{{{
==156518== Invalid read of size 1
==156518==    at 0x413BE0: vfs_parse_ls_lga (parse_ls_vga.c:863)
==156518==    by 0x4076C3: process_ls_line (mc_parse_ls_l.c:350)
==156518==    by 0x4076C3: process_input (mc_parse_ls_l.c:376)
==156518==    by 0x40736B: main (mc_parse_ls_l.c:404)
==156518==  Address 0x536be6f is 1 bytes before a block of size 2 alloc'd
==156518==    at 0x48854F0: malloc (vg_replace_malloc.c:446)
==156518==    by 0x4CF4FCB: g_malloc (gmem.c:100)
==156518==    by 0x4D0E99B: g_strdup (gstrfuncs.c:323)
==156518==    by 0x413887: g_strdup_inline (gstrfuncs.h:321)
==156518==    by 0x413887: vfs_parse_ls_lga (parse_ls_vga.c:848)
==156518==    by 0x4076C3: process_ls_line (mc_parse_ls_l.c:350)
==156518==    by 0x4076C3: process_input (mc_parse_ls_l.c:376)
==156518==    by 0x40736B: main (mc_parse_ls_l.c:404)
}}}

https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/79071

Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

lib/strutil.h

@@ -578,6 +578,8 @@ char *str_regex_unescape (const char *text);
 
 gboolean str_is_char_escaped (const char *start, const char *current);
 
+void str_rstrip_eol (char *s);
+
 /* --------------------------------------------------------------------------------------------- */
 /*** inline functions ****************************************************************************/
 /* --------------------------------------------------------------------------------------------- */

lib/strutil/strutil.c

@@ -1020,3 +1020,21 @@ parse_integer (const char *str, gboolean *invalid)
 }
 
 /* --------------------------------------------------------------------------------------------- */
+
+/**
+ * Strips single right (trailing) EOL (\\r, \\n, or \\r\\n), NULL-safe
+ */
+void
+str_rstrip_eol (char *s)
+{
+    if (s == NULL || *s == '\0')
+        return;
+
+    const size_t len = strlen (s);
+    if (len >= 2 && s[len - 2] == '\r' && s[len - 1] == '\n')  // removes \r\n
+        s[len - 2] = '\0';
+    else if (len >= 1 && (s[len - 1] == '\n' || s[len - 1] == '\r'))  // removes \n or \r
+        s[len - 1] = '\0';
+}
+
+/* --------------------------------------------------------------------------------------------- */

lib/vfs/parse_ls_vga.c

@@ -40,6 +40,7 @@
 #include <stdlib.h>
 
 #include "lib/global.h"
+#include "lib/strutil.h"
 #include "lib/unixcompat.h"  // makedev
 #include "lib/widget.h"      // message()
 
@@ -852,16 +853,7 @@ vfs_parse_ls_lga (const char *p, struct stat *s, char **filename, char **linknam
             *linkname = NULL;
     }
 
-    if (t != NULL)
-    {
-        size_t p2;
-
-        p2 = strlen (t);
-        if (--p2 > 0 && (t[p2] == '\r' || t[p2] == '\n'))
-            t[p2] = '\0';
-        if (--p2 > 0 && (t[p2] == '\r' || t[p2] == '\n'))
-            t[p2] = '\0';
-    }
+    str_rstrip_eol (t);
 
     g_free (p_copy);
     return TRUE;

tests/lib/strutil/Makefile.am

@@ -16,6 +16,7 @@ TESTS = \
 	parse_integer \
 	str_replace_all \
 	str_verscmp \
+	str_rstrip_eol \
 	filevercmp
 
 check_PROGRAMS = $(TESTS)
@@ -29,5 +30,8 @@ str_replace_all_SOURCES = \
 str_verscmp_SOURCES = \
 	str_verscmp.c
 
+str_rstrip_eol_SOURCES = \
+	str_rstrip_eol.c
+
 filevercmp_SOURCES = \
 	filevercmp.c

tests/lib/strutil/str_rstrip_eol.c

@@ -0,0 +1,128 @@
+/*
+   lib/strutil - tests for lib/strutil.c:str_rstrip_eol function
+
+   Copyright (C) 2025
+   Free Software Foundation, Inc.
+
+   This file is part of the Midnight Commander.
+
+   The Midnight Commander is free software: you can redistribute it
+   and/or modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation, either version 3 of the License,
+   or (at your option) any later version.
+
+   The Midnight Commander is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#define TEST_SUITE_NAME "/lib/strutil"
+
+#include "tests/mctest.h"
+
+#include "lib/strutil.h"
+
+/* --------------------------------------------------------------------------------------------- */
+
+/* @Before */
+static void
+setup (void)
+{
+}
+
+/* --------------------------------------------------------------------------------------------- */
+
+/* @After */
+static void
+teardown (void)
+{
+}
+
+/* --------------------------------------------------------------------------------------------- */
+
+/* @DataSource("str_rstrip_eol_test_ds1") */
+/* Testcases are taken from Glib */
+static const struct str_rstrip_eol_test_struct
+{
+    const char *input_string;
+    const char *expected_result;
+} str_rstrip_eol_test_ds1[] = {
+    {
+        "",
+        "",
+    },
+    {
+        " \n\r",
+        " \n",
+    },
+    {
+        " \t\r\n",
+        " \t",
+    },
+    {
+        "a \r ",
+        "a \r ",
+    },
+    {
+        " a  \n ",
+        " a  \n ",
+    },
+    {
+        "a a\n\r\n",
+        "a a\n",
+    },
+    {
+        "\na a \r",
+        "\na a ",
+    },
+};
+
+/* @Test(dataSource = "str_rstrip_eol_test_ds1") */
+START_TEST (str_rstrip_eol_test1)
+{
+    /* given */
+    const struct str_rstrip_eol_test_struct *data = &str_rstrip_eol_test_ds1[_i];
+
+    /* when */
+    char *actual_result = g_strdup (data->input_string);
+    str_rstrip_eol (actual_result);
+
+    /* then */
+    ck_assert_str_eq (actual_result, data->expected_result);
+
+    g_free (actual_result);
+}
+
+END_TEST
+/* --------------------------------------------------------------------------------------------- */
+START_TEST (str_rstrip_eol_test_null)
+{
+    char *ptr = NULL;
+    str_rstrip_eol (ptr);
+    ck_assert_ptr_null (ptr);
+}
+
+END_TEST
+/* --------------------------------------------------------------------------------------------- */
+int
+main (void)
+{
+    TCase *tc_core;
+
+    tc_core = tcase_create ("Core");
+
+    tcase_add_checked_fixture (tc_core, setup, teardown);
+
+    /* Add new tests here: *************** */
+    mctest_add_parameterized_test (tc_core, str_rstrip_eol_test1, str_rstrip_eol_test_ds1);
+    tcase_add_test (tc_core, str_rstrip_eol_test_null);
+    /* *********************************** */
+
+    return mctest_run_all (tc_core);
+}
+
+/* --------------------------------------------------------------------------------------------- */

tests/src/vfs/extfs/helpers-list/mc_parse_ls_l.c

@@ -42,8 +42,9 @@
 
 #include "lib/vfs/utilvfs.h"  // vfs_parse_ls_lga()
 #include "lib/util.h"         // string_perm()
-#include "lib/timefmt.h"      // FMT_LOCALTIME
-#include "lib/widget.h"       // for the prototype of message() only
+#include "lib/strutil.h"
+#include "lib/timefmt.h"  // FMT_LOCALTIME
+#include "lib/widget.h"   // for the prototype of message() only
 
 /*** global variables ****************************************************************************/
 
@@ -200,25 +201,6 @@ symbolic_gid (gid_t gid)
 
 /* --------------------------------------------------------------------------------------------- */
 
-/**
- * Cuts off a string's line-end (as in Perl).
- */
-static void
-chomp (char *s)
-{
-    int i;
-
-    i = strlen (s);
-
-    // Code taken from vfs_parse_ls_lga(), with modifications.
-    if ((--i >= 0) && (s[i] == '\r' || s[i] == '\n'))
-        s[i] = '\0';
-    if ((--i >= 0) && (s[i] == '\r' || s[i] == '\n'))
-        s[i] = '\0';
-}
-
-/* --------------------------------------------------------------------------------------------- */
-
 static const char *
 string_date (time_t t)
 {
@@ -396,7 +378,7 @@ process_input (FILE *input)
 
     while (fgets (line, sizeof line, input) != NULL)
     {
-        chomp (line);                          // Not mandatory. Makes error messages nicer.
+        str_rstrip_eol (line);                 // Not mandatory. Makes error messages nicer.
         if (strncmp (line, "total ", 6) == 0)  // Convenience only: makes 'ls -l' parse cleanly.
             continue;
         process_ls_line (line);