[PATCH v2] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083]

[PATCH v2] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083]

[Siddhesh Poyarekar]

The addmntent function replicates elements of struct mnt on stack
using alloca, which is unsafe.  Put characters directly into the
stream, escaping them as they're being written out.

Also add a test to check all escaped characters with addmntent and
getmntent.
---
 misc/Makefile            |   2 +-
 misc/mntent_r.c          | 113 ++++++++++++++-------------------------
 misc/tst-mntent-escape.c | 101 ++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 74 deletions(-)
 create mode 100644 misc/tst-mntent-escape.c

diff --git a/misc/Makefile b/misc/Makefile
index 58959f6913..92816af2a2 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -88,7 +88,7 @@ tests := tst-dirname tst-tsearch tst-fdset tst-mntent tst-hsearch \
 	 tst-preadvwritev tst-preadvwritev64 tst-makedev tst-empty \
 	 tst-preadvwritev2 tst-preadvwritev64v2 tst-warn-wide \
 	 tst-ldbl-warn tst-ldbl-error tst-dbl-efgcvt tst-ldbl-efgcvt \
-	 tst-mntent-autofs tst-syscalls
+	 tst-mntent-autofs tst-syscalls tst-mntent-escape
 
 # Tests which need libdl.
 ifeq (yes,$(build-shared))
diff --git a/misc/mntent_r.c b/misc/mntent_r.c
index 90a73f2dda..ac5a83d1cf 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -232,87 +232,54 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
 libc_hidden_def (__getmntent_r)
 weak_alias (__getmntent_r, getmntent_r)
 
+/* Write STR into STREAM, escaping whitespaces as we go.  */
+#define PUTC_OR_FAIL(_c, _st) if (fputc_unlocked ((_c), (_st)) < 0) return -1
+static int
+write_string (FILE *stream, const char *str)
+{
+  char c;
+  const char *encode_chars = " \t\n\\";
 
-/* We have to use an encoding for names if they contain spaces or tabs.
-   To be able to represent all characters we also have to escape the
-   backslash itself.  This "function" must be a macro since we use
-   `alloca'.  */
-#define encode_name(name) \
-  do {									      \
-    const char *rp = name;						      \
-									      \
-    while (*rp != '\0')							      \
-      if (*rp == ' ' || *rp == '\t' || *rp == '\n' || *rp == '\\')	      \
-	break;								      \
-      else								      \
-	++rp;								      \
-									      \
-    if (*rp != '\0')							      \
-      {									      \
-	/* In the worst case the length of the string can increase to	      \
-	   four times the current length.  */				      \
-	char *wp;							      \
-									      \
-	rp = name;							      \
-	name = wp = (char *) alloca (strlen (name) * 4 + 1);		      \
-									      \
-	do								      \
-	  if (*rp == ' ')						      \
-	    {								      \
-	      *wp++ = '\\';						      \
-	      *wp++ = '0';						      \
-	      *wp++ = '4';						      \
-	      *wp++ = '0';						      \
-	    }								      \
-	  else if (*rp == '\t')						      \
-	    {								      \
-	      *wp++ = '\\';						      \
-	      *wp++ = '0';						      \
-	      *wp++ = '1';						      \
-	      *wp++ = '1';						      \
-	    }								      \
-	  else if (*rp == '\n')						      \
-	    {								      \
-	      *wp++ = '\\';						      \
-	      *wp++ = '0';						      \
-	      *wp++ = '1';						      \
-	      *wp++ = '2';						      \
-	    }								      \
-	  else if (*rp == '\\')						      \
-	    {								      \
-	      *wp++ = '\\';						      \
-	      *wp++ = '\\';						      \
-	    }								      \
-	  else								      \
-	    *wp++ = *rp;						      \
-	while (*rp++ != '\0');						      \
-      }									      \
-  } while (0)
-
+  while ((c = *str++) != '\0')
+    {
+      if (strchr (encode_chars, c) == NULL)
+	{
+	  PUTC_OR_FAIL (c, stream);
+	}
+      else
+	{
+	  PUTC_OR_FAIL ('\\', stream);
+	  PUTC_OR_FAIL (((c & 0xc0) >> 6) + '0', stream);
+	  PUTC_OR_FAIL (((c & 0x38) >> 3) + '0', stream);
+	  PUTC_OR_FAIL (((c & 0x07) >> 0) + '0', stream);
+	}
+    }
+  PUTC_OR_FAIL (' ', stream);
+  return 0;
+}
 
 /* Write the mount table entry described by MNT to STREAM.
    Return zero on success, nonzero on failure.  */
 int
 __addmntent (FILE *stream, const struct mntent *mnt)
 {
-  struct mntent mntcopy = *mnt;
+  int ret = 1;
+
   if (fseek (stream, 0, SEEK_END))
-    return 1;
-
-  /* Encode spaces and tabs in the names.  */
-  encode_name (mntcopy.mnt_fsname);
-  encode_name (mntcopy.mnt_dir);
-  encode_name (mntcopy.mnt_type);
-  encode_name (mntcopy.mnt_opts);
-
-  return (fprintf (stream, "%s %s %s %s %d %d\n",
-		   mntcopy.mnt_fsname,
-		   mntcopy.mnt_dir,
-		   mntcopy.mnt_type,
-		   mntcopy.mnt_opts,
-		   mntcopy.mnt_freq,
-		   mntcopy.mnt_passno) < 0
-	  || fflush (stream) != 0);
+    return ret;
+
+  flockfile (stream);
+
+  ret = (write_string (stream, mnt->mnt_fsname) < 0
+	 || write_string (stream, mnt->mnt_dir) < 0
+	 || write_string (stream, mnt->mnt_type) < 0
+	 || write_string (stream, mnt->mnt_opts) < 0
+	 || fprintf (stream, "%d %d\n", mnt->mnt_freq, mnt->mnt_passno) < 0
+	 || fflush (stream) != 0);
+
+  funlockfile (stream);
+
+  return ret;
 }
 weak_alias (__addmntent, addmntent)
 
diff --git a/misc/tst-mntent-escape.c b/misc/tst-mntent-escape.c
new file mode 100644
index 0000000000..c1db428a9d
--- /dev/null
+++ b/misc/tst-mntent-escape.c
@@ -0,0 +1,101 @@
+/* Test mntent interface with escaped sequences.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <mntent.h>
+#include <stdio.h>
+#include <string.h>
+#include <support/check.h>
+
+struct const_mntent
+{
+  const char *mnt_fsname;
+  const char *mnt_dir;
+  const char *mnt_type;
+  const char *mnt_opts;
+  int mnt_freq;
+  int mnt_passno;
+  const char *expected;
+};
+
+struct const_mntent tests[] =
+{
+    {"/dev/hda1", "/some dir", "ext2", "defaults", 1, 2,
+     "/dev/hda1 /some\\040dir ext2 defaults 1 2\n"},
+    {"device name", "/some dir", "tmpfs", "defaults", 1, 2,
+     "device\\040name /some\\040dir tmpfs defaults 1 2\n"},
+    {" ", "/some dir", "tmpfs", "defaults", 1, 2,
+     "\\040 /some\\040dir tmpfs defaults 1 2\n"},
+    {"\t", "/some dir", "tmpfs", "defaults", 1, 2,
+     "\\011 /some\\040dir tmpfs defaults 1 2\n"},
+    {"\\", "/some dir", "tmpfs", "defaults", 1, 2,
+     "\\134 /some\\040dir tmpfs defaults 1 2\n"},
+};
+
+static int
+do_test (void)
+{
+  for (int i = 0; i < sizeof (tests) / sizeof (struct const_mntent); i++)
+    {
+      char buf[128];
+      struct mntent *ret, curtest;
+      FILE *fp = fmemopen (buf, sizeof (buf), "w+");
+
+      if (fp == NULL)
+	{
+	  printf ("Failed to open file\n");
+	  return 1;
+	}
+
+      curtest.mnt_fsname = strdupa (tests[i].mnt_fsname);
+      curtest.mnt_dir = strdupa (tests[i].mnt_dir);
+      curtest.mnt_type = strdupa (tests[i].mnt_type);
+      curtest.mnt_opts = strdupa (tests[i].mnt_opts);
+      curtest.mnt_freq = tests[i].mnt_freq;
+      curtest.mnt_passno = tests[i].mnt_passno;
+
+      if (addmntent (fp, &curtest) != 0)
+	{
+	  support_record_failure ();
+	  continue;
+	}
+
+      TEST_COMPARE_STRING (buf, tests[i].expected);
+
+      rewind (fp);
+      ret = getmntent (fp);
+      if (ret == NULL)
+	{
+	  support_record_failure ();
+	  continue;
+	}
+
+      TEST_COMPARE_STRING(tests[i].mnt_fsname, ret->mnt_fsname);
+      TEST_COMPARE_STRING(tests[i].mnt_dir, ret->mnt_dir);
+      TEST_COMPARE_STRING(tests[i].mnt_type, ret->mnt_type);
+      TEST_COMPARE_STRING(tests[i].mnt_opts, ret->mnt_opts);
+      TEST_COMPARE(tests[i].mnt_freq, ret->mnt_freq);
+      TEST_COMPARE(tests[i].mnt_passno, ret->mnt_passno);
+
+      fclose (fp);
+    }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.29.2

Re: [PATCH v2] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083]

[Florian Weimer]

* Siddhesh Poyarekar via Libc-alpha:

> +/* Write STR into STREAM, escaping whitespaces as we go.  */
> +#define PUTC_OR_FAIL(_c, _st) if (fputc_unlocked ((_c), (_st)) < 0) return -1

I think you should just keep writing on ferror and check for an error
using ferror in __addmntent.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH v2] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083]

[Szabolcs Nagy]

The 12/22/2020 15:38, Siddhesh Poyarekar via Libc-alpha wrote:
> The addmntent function replicates elements of struct mnt on stack
> using alloca, which is unsafe.  Put characters directly into the
> stream, escaping them as they're being written out.
> 
> Also add a test to check all escaped characters with addmntent and
> getmntent.

aarch64 buildbot fails because

$ cat elf/check-localplt.out
Extra PLT reference: libc.so: ferror

Re: [PATCH v2] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083]

[Florian Weimer]

* Szabolcs Nagy:

> The 12/22/2020 15:38, Siddhesh Poyarekar via Libc-alpha wrote:
>> The addmntent function replicates elements of struct mnt on stack
>> using alloca, which is unsafe.  Put characters directly into the
>> stream, escaping them as they're being written out.
>> 
>> Also add a test to check all escaped characters with addmntent and
>> getmntent.
>
> aarch64 buildbot fails because
>
> $ cat elf/check-localplt.out
> Extra PLT reference: libc.so: ferror

Looking at include/stdio.h, __ferror_unlocked should work instead and
avoid the PLT reference (it's either inlined, or results in a hidden
reference).

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

[COMMITTED] misc: Use __ferror_unlocked instead of ferror

[Siddhesh Poyarekar]

The ferror results in an unnecessary PLT reference.  Use
__ferror_unlocked instead , which gets inlined.
---
 misc/mntent_r.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/mntent_r.c b/misc/mntent_r.c
index 39fa08f2c6..b963c9af61 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -253,7 +253,7 @@ __addmntent (FILE *stream, const struct mntent *mnt)
   write_string (stream, mnt->mnt_opts);
   fprintf (stream, "%d %d\n", mnt->mnt_freq, mnt->mnt_passno);
 
-  ret = ferror (stream) != 0 || fflush (stream) != 0;
+  ret = __ferror_unlocked (stream) != 0 || fflush (stream) != 0;
 
   funlockfile (stream);
 
-- 
2.29.2