diff mbox

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

Message ID CALoOobMKYAvdj63=7JKyQTPZHx_6c_0RVTxXjcLk-_wyFYWryg@mail.gmail.com
State Superseded
Headers

Commit Message

Paul Pluzhnikov Feb. 22, 2015, 11:31 p.m. UTC 
  On Sun, Feb 22, 2015 at 12:59 PM, Florian Weimer <fw@deneb.enyo.de> wrote:

Thanks for the review.

>> Attached patch fixes BZ #17916 fopen unbounded stack usage for ccs= modes
>> Tested on Linux/x86_64, no failures.
>
> This should have a test case.

I am not sure how to test for unbounded stack use.
I've added a test for humungous ccs= value.

> There is a missing NULL check on the malloc result.  I don't think the
> alloca optimization makes sense because this functionality is used so
> rarely, and the function allocates on the heap anyway.  Just use
> malloc unconditionally; it will result in smaller code.

Done.

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): Add a large ccs= test

Comments

Florian Weimer Feb. 23, 2015, 1:47 p.m. UTC | #1 
On 02/23/2015 12:31 AM, Paul Pluzhnikov wrote:
> +	  if (ccs == NULL)
> +	    return NULL;

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

Test case change is okay, this is how we usually test for stack overflow.
Joseph Myers Feb. 23, 2015, 4:33 p.m. UTC | #2 
On Sun, 22 Feb 2015, Paul Pluzhnikov wrote:

> On Sun, Feb 22, 2015 at 12:59 PM, Florian Weimer <fw@deneb.enyo.de> wrote:
> 
> Thanks for the review.
> 
> >> Attached patch fixes BZ #17916 fopen unbounded stack usage for ccs= modes
> >> Tested on Linux/x86_64, no failures.
> >
> > This should have a test case.
> 
> I am not sure how to test for unbounded stack use.

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

Patch

diff --git a/NEWS b/NEWS
index 28ef45d..660fbe0 100644
--- a/NEWS
+++ b/NEWS
@@ -10,8 +10,8 @@  Version 2.22
 * The following bugs are resolved with this release:
 
   4719, 13064, 14094, 15319, 15467, 15790, 16560, 17269, 17569, 17588,
-  17792, 17912, 17932, 17944, 17949, 17964, 17965, 17967, 17969, 17978,
-  17987, 17991, 17996, 17998, 17999.
+  17792, 17912, 17916, 17932, 17944, 17949, 17964, 17965, 17967, 17969,
+  17978, 17987, 17991, 17996, 17998, 17999.
 
 * Character encoding and ctype tables were updated to Unicode 7.0.0, using
   new generator scripts contributed by Pravin Satpute and Mike FABIAN (Red
diff --git a/libio/fileops.c b/libio/fileops.c
index 297b478..c6ad2cf 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -353,7 +353,10 @@  _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)
+	    return NULL;
 
 	  *((char *) __mempcpy (ccs, cs + 5, endp - (cs + 5))) = '\0';
 	  strip (ccs, ccs);
@@ -365,10 +368,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..ddabc0a 100644
--- a/libio/tst-fopenloc.c
+++ b/libio/tst-fopenloc.c
@@ -57,6 +57,21 @@  do_test (void)
 
   fclose (fp);
 
+  /* BZ #17916 -- check invalid large ccs= case.  */
+  const size_t sz = 1 << 24;  /* 16MiB  */
+  char *ccs = malloc (sz);
+  strcpy (ccs, "r,ccs=");
+  memset (ccs + 6, 'A', sz - 6 - 1);
+  ccs[sz - 1] = '\0';
+
+  fp = fopen (inputfile, ccs);
+  if (fp != NULL)
+    {
+      printf ("unxpected success\n");
+      return 1;
+    }
+  free (ccs);
+
   return 0;
 }