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

Message ID 20201222115140.2055339-1-siddhesh@sourceware.org
State Committed
Headers
Series [v3] addmntent: Remove unbounded alloca usage from getmntent [BZ#27083] |

Commit Message

Siddhesh Poyarekar Dec. 22, 2020, 11:51 a.m. UTC
  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
  

Comments

Florian Weimer Dec. 22, 2020, 2:57 p.m. UTC | #1
* 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
  
Stefan Liebler Jan. 8, 2021, 12:09 p.m. UTC | #2
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
  
Siddhesh Poyarekar Jan. 8, 2021, 1:34 p.m. UTC | #3
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
  

Patch

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>