diff mbox

Fix BZ #17916 fopen unbounded stack usage for ccs= modes

Message ID CALoOobOsHaTWbf9xKjLB6TxbtUvnT4v704Y+pixbra8ODrJaFw@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Feb. 24, 2015, 5 a.m. UTC 
On Mon, Feb 23, 2015 at 8:33 AM, Joseph Myers <joseph@codesourcery.com> wrote:

> Typically such tests use setrlimit to set a stack limit lower than the
> amount of stack space the code used before the fix.

Thanks. I've updated the test and verified that it fails with stack
overflow if I revert the fix.

On Mon, Feb 23, 2015 at 5:47 AM, Florian Weimer <fweimer@redhat.com> wrote:

> I think you have to call _IO_file_close_it (fp) here, otherwise there's
> a resource leak.

Thanks. Fixed.


2015-02-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

	    [BZ #17916]
	    * libio/fileops.c (_IO_new_file_fopen): Limit stack use
	    * libio/tst-fopenloc.c (do_test, do_bz17916): Add a large ccs= test

Comments

Florian Weimer Feb. 24, 2015, 9:47 a.m. UTC | #1 
On 02/24/2015 06:00 AM, Paul Pluzhnikov wrote:
> On Mon, Feb 23, 2015 at 8:33 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> 
>> Typically such tests use setrlimit to set a stack limit lower than the
>> amount of stack space the code used before the fix.
> 
> Thanks. I've updated the test and verified that it fails with stack
> overflow if I revert the fix.

> 2015-02-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	    [BZ #17916]
> 	    * libio/fileops.c (_IO_new_file_fopen): Limit stack use
> 	    * libio/tst-fopenloc.c (do_test, do_bz17916): Add a large ccs= test

Thanks, this is okay to commit.
Roland McGrath Feb. 25, 2015, 8:53 p.m. UTC | #2 
> +static
> +int do_bz17916 (void)

GNU style is that the function's name is the first thing on the line.
Everything else goes before that line.
Paul Pluzhnikov Feb. 25, 2015, 9:12 p.m. UTC | #3 
On Wed, Feb 25, 2015 at 12:53 PM, Roland McGrath <roland@hack.frob.com> wrote:
>
> > +static
> > +int do_bz17916 (void)
>
> GNU style is that the function's name is the first thing on the line.
> Everything else goes before that line.

Oops. I knew that. Sorry.
Fixed in d68eba500ca0347364b4c428c51fb607c9278f07.


--
Paul Pluzhnikov
diff mbox

Patch

diff --git a/libio/fileops.c b/libio/fileops.c
index 297b478..2427320 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -353,7 +353,15 @@  _IO_new_file_fopen (_IO_FILE *fp, const char *filename, const char *mode,
 	  struct gconv_fcts fcts;
 	  struct _IO_codecvt *cc;
 	  char *endp = __strchrnul (cs + 5, ',');
-	  char ccs[endp - (cs + 5) + 3];
+	  char *ccs = malloc (endp - (cs + 5) + 3);
+
+	  if (ccs == NULL)
+	    {
+	      int malloc_err = errno;  /* Whatever malloc failed with.  */
+	      (void) _IO_file_close_it (fp);
+	      __set_errno (malloc_err);
+	      return NULL;
+	    }
 
 	  *((char *) __mempcpy (ccs, cs + 5, endp - (cs + 5))) = '\0';
 	  strip (ccs, ccs);
@@ -365,10 +373,13 @@  _IO_new_file_fopen (_IO_FILE *fp, const char *filename, const char *mode,
 		 This means we cannot proceed since the user explicitly asked
 		 for these.  */
 	      (void) _IO_file_close_it (fp);
+	      free (ccs);
 	      __set_errno (EINVAL);
 	      return NULL;
 	    }
 
+	  free (ccs);
+
 	  assert (fcts.towc_nsteps == 1);
 	  assert (fcts.tomb_nsteps == 1);
 
diff --git a/libio/tst-fopenloc.c b/libio/tst-fopenloc.c
index 1336023..48c2d3b 100644
--- a/libio/tst-fopenloc.c
+++ b/libio/tst-fopenloc.c
@@ -24,10 +24,36 @@ 
 #include <stdlib.h>
 #include <string.h>
 #include <wchar.h>
+#include <sys/resource.h>
 
 
 static const char inputfile[] = "../iconvdata/testdata/ISO-8859-1";
 
+static
+int do_bz17916 (void)
+{
+  /* BZ #17916 -- check invalid large ccs= case.  */
+  struct rlimit rl;
+  getrlimit (RLIMIT_STACK, &rl);
+  rl.rlim_cur = 1024 * 1024;
+  setrlimit (RLIMIT_STACK, &rl);
+
+  const size_t sz = 2 * 1024 * 1024;
+  char *ccs = malloc (sz);
+  strcpy (ccs, "r,ccs=");
+  memset (ccs + 6, 'A', sz - 6 - 1);
+  ccs[sz - 1] = '\0';
+
+  FILE *fp = fopen (inputfile, ccs);
+  if (fp != NULL)
+    {
+      printf ("unxpected success\n");
+      return 1;
+    }
+  free (ccs);
+
+  return 0;
+}
 
 static int
 do_test (void)
@@ -57,7 +83,7 @@  do_test (void)
 
   fclose (fp);
 
-  return 0;
+  return do_bz17916 ();
 }
 
 #define TEST_FUNCTION do_test ()