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

[PATCH v3] 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.
---
Changes since last version:

- Check for stream error only once, before flushing.

 misc/Makefile            |   2 +-
 misc/mntent_r.c          | 111 ++++++++++++++-------------------------
 misc/tst-mntent-escape.c | 101 +++++++++++++++++++++++++++++++++++
 3 files changed, 140 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..c9148c5b5e 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -232,87 +232,52 @@ __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.  Do not check for
+   errors here; we check the stream status in __ADDMNTENT.  */
+static void
+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)
+	fputc_unlocked (c, stream);
+      else
+	{
+	  fputc_unlocked ('\\', stream);
+	  fputc_unlocked (((c & 0xc0) >> 6) + '0', stream);
+	  fputc_unlocked (((c & 0x38) >> 3) + '0', stream);
+	  fputc_unlocked (((c & 0x07) >> 0) + '0', stream);
+	}
+    }
+  fputc_unlocked (' ', stream);
+}
 
 /* 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);
+
+  write_string (stream, mnt->mnt_fsname);
+  write_string (stream, mnt->mnt_dir);
+  write_string (stream, mnt->mnt_type);
+  write_string (stream, mnt->mnt_opts);
+  fprintf (stream, "%d %d\n", mnt->mnt_freq, mnt->mnt_passno);
+
+  ret = ferror (stream) != 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 v3] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083]

[Florian Weimer]

* Siddhesh Poyarekar via Libc-alpha:

> 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.

This version looks okay to me.

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 v3] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083]

[Stefan Liebler]

On 12/22/20 3:57 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar via Libc-alpha:
> 
>> 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.
> 
> This version looks okay to me.
> 
> Thanks,
> Florian
> 

Hi Siddhesh,

starting with this patch, I've observed lots of linknamespace errors if
build with -Os:
FAIL: conform/POSIX/semaphore.h/linknamespace
FAIL: conform/POSIX/sys/mman.h/linknamespace
FAIL: conform/POSIX/unistd.h/linknamespace
FAIL: conform/POSIX2008/semaphore.h/linknamespace
FAIL: conform/POSIX2008/sys/mman.h/linknamespace
FAIL: conform/POSIX2008/unistd.h/linknamespace
FAIL: conform/UNIX98/semaphore.h/linknamespace
FAIL: conform/UNIX98/sys/mman.h/linknamespace
FAIL: conform/UNIX98/unistd.h/linknamespace
FAIL: conform/XOPEN2K/semaphore.h/linknamespace
FAIL: conform/XOPEN2K/sys/mman.h/linknamespace
FAIL: conform/XOPEN2K/unistd.h/linknamespace
FAIL: conform/XOPEN2K8/semaphore.h/linknamespace
FAIL: conform/XOPEN2K8/sys/mman.h/linknamespace
FAIL: conform/XOPEN2K8/unistd.h/linknamespace
FAIL: conform/XPG4/unistd.h/linknamespace
FAIL: conform/XPG42/unistd.h/linknamespace

E.g.:
conform/POSIX/semaphore.h/linknamespace.out
[initial] sem_open -> [libpthread.a(sem_open.o)] __shm_directory ->
[libpthread.a(shm-directory.o)] __endmntent -> [libc.a(mntent_r.o)]
fputc_unlocked

Can you please have a look?

Bye,
Stefan

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

[Siddhesh Poyarekar]

On 1/8/21 5:39 PM, Stefan Liebler wrote:
> starting with this patch, I've observed lots of linknamespace errors if
> build with -Os:
> FAIL: conform/POSIX/semaphore.h/linknamespace
> FAIL: conform/POSIX/sys/mman.h/linknamespace
> FAIL: conform/POSIX/unistd.h/linknamespace
> FAIL: conform/POSIX2008/semaphore.h/linknamespace
> FAIL: conform/POSIX2008/sys/mman.h/linknamespace
> FAIL: conform/POSIX2008/unistd.h/linknamespace
> FAIL: conform/UNIX98/semaphore.h/linknamespace
> FAIL: conform/UNIX98/sys/mman.h/linknamespace
> FAIL: conform/UNIX98/unistd.h/linknamespace
> FAIL: conform/XOPEN2K/semaphore.h/linknamespace
> FAIL: conform/XOPEN2K/sys/mman.h/linknamespace
> FAIL: conform/XOPEN2K/unistd.h/linknamespace
> FAIL: conform/XOPEN2K8/semaphore.h/linknamespace
> FAIL: conform/XOPEN2K8/sys/mman.h/linknamespace
> FAIL: conform/XOPEN2K8/unistd.h/linknamespace
> FAIL: conform/XPG4/unistd.h/linknamespace
> FAIL: conform/XPG42/unistd.h/linknamespace
> 
> E.g.:
> conform/POSIX/semaphore.h/linknamespace.out
> [initial] sem_open -> [libpthread.a(sem_open.o)] __shm_directory ->
> [libpthread.a(shm-directory.o)] __endmntent -> [libc.a(mntent_r.o)]
> fputc_unlocked
> 
> Can you please have a look?

Hmm, I guess fputc_unlocked doesn't get inlined with -Os.  I'll take a look.

Siddhesh