diff mbox

Fix BZ #18757 (fmemopen doesn't fail with invalid modes)

Message ID CALoOobNGvmPXiAGuL5XfeUaf-ueMB-Y7NvtEWgFPpVTv0EW=vA@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Aug. 10, 2015, 3:45 a.m. UTC 
On Sun, Aug 9, 2015 at 7:36 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 09 Aug 2015 15:28, Paul Pluzhnikov wrote:
>> +/* Check valid open mode.  Only "(r|w|a)\+?" are valid.  */
>
> this is not what the documentation states:

AFAICT, only "r", "w", "w+" and "a" are really used by __fmemopen, but
I overlooked the _IO_fopencookie part.

> https://www.gnu.org/software/libc/manual/html_node/String-Streams.html
>         "The argument opentype is the same as in fopen"
> https://www.gnu.org/software/libc/manual/html_node/Opening-Streams.html
>         there are various GNU extensions here, and the standard 'b' flag

Handling of 'b' in fmemopen has been removed:

https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=fdb7d390dd0d96e4a8239c46f3aa64598b90842b
I also filed https://bugzilla.kernel.org/show_bug.cgi?id=102551 to
update man7.org description ...


> so the only thing we should check is whether the first byte is [rwa].  and it
> seems to me that _IO_fopencookie (which fmemopen tails into) already does just
> that for us.

Oh, I see. The bug is that _IO_fopencookie doesn't set errno.
In addition, if _IO_fopencookie fails, we leak memory.

Revised patch attached.


2015-08-09  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #18757]
        * libio/fmemopen.c (__fmemopen): Set EINVAL and don't leak memory.
        * libio/oldfmemopen.c (__old_fmemopen): Likewise.
        * libio/test-fmemopen.c (do_bz18757): New test.

Comments

Mike Frysinger Aug. 10, 2015, 4:15 a.m. UTC | #1 
On 09 Aug 2015 20:45, Paul Pluzhnikov wrote:
> On Sun, Aug 9, 2015 at 7:36 PM, Mike Frysinger wrote:
> > On 09 Aug 2015 15:28, Paul Pluzhnikov wrote:
> >> +/* Check valid open mode.  Only "(r|w|a)\+?" are valid.  */
> >
> > this is not what the documentation states:
> 
> AFAICT, only "r", "w", "w+" and "a" are really used by __fmemopen,

i'm not debating what the code actually does, just that your proposed changes
violate the documentation and the guarantees we've made about this API in the
past.  i think that alone means we shouldn't try to tighten down things (at
least beyond what fopen does).

> --- a/libio/fmemopen.c
> +++ b/libio/fmemopen.c
> @@ -149,6 +149,7 @@ __fmemopen (void *buf, size_t len, const char *mode)
>  {
>    cookie_io_functions_t iof;
>    fmemopen_cookie_t *c;
> +  FILE *ret;
>  
>    c = (fmemopen_cookie_t *) calloc (sizeof (fmemopen_cookie_t), 1);
>    if (c == NULL)
> @@ -209,7 +210,16 @@ __fmemopen (void *buf, size_t len, const char *mode)
>    iof.seek = fmemopen_seek;
>    iof.close = fmemopen_close;
>  
> -  return _IO_fopencookie (c, mode, iof);
> +  ret = _IO_fopencookie (c, mode, iof);
> +
> +  if (__glibc_unlikely (ret == NULL))
> +    {
> +      /* BZ #18757 -- set EINVAL  */

should have a period at the end.

> +      __set_errno (EINVAL);
> +      free (c);

i think you also need to free c->buffer when c->mybuffer is true
-mike
diff mbox

Patch

diff --git a/libio/fmemopen.c b/libio/fmemopen.c
index 3ab3e8d..1addfac 100644
--- a/libio/fmemopen.c
+++ b/libio/fmemopen.c
@@ -149,6 +149,7 @@  __fmemopen (void *buf, size_t len, const char *mode)
 {
   cookie_io_functions_t iof;
   fmemopen_cookie_t *c;
+  FILE *ret;
 
   c = (fmemopen_cookie_t *) calloc (sizeof (fmemopen_cookie_t), 1);
   if (c == NULL)
@@ -209,7 +210,16 @@  __fmemopen (void *buf, size_t len, const char *mode)
   iof.seek = fmemopen_seek;
   iof.close = fmemopen_close;
 
-  return _IO_fopencookie (c, mode, iof);
+  ret = _IO_fopencookie (c, mode, iof);
+
+  if (__glibc_unlikely (ret == NULL))
+    {
+      /* BZ #18757 -- set EINVAL  */
+      __set_errno (EINVAL);
+      free (c);
+    }
+
+  return ret;
 }
 libc_hidden_def (__fmemopen)
 versioned_symbol (libc, __fmemopen, fmemopen, GLIBC_2_22);
diff --git a/libio/oldfmemopen.c b/libio/oldfmemopen.c
index 8e35672..40432d1 100644
--- a/libio/oldfmemopen.c
+++ b/libio/oldfmemopen.c
@@ -204,6 +204,7 @@  __old_fmemopen (void *buf, size_t len, const char *mode)
 {
   cookie_io_functions_t iof;
   fmemopen_cookie_t *c;
+  FILE *ret;
 
   if (__glibc_unlikely (len == 0))
     {
@@ -259,7 +260,15 @@  __old_fmemopen (void *buf, size_t len, const char *mode)
   iof.seek = fmemopen_seek;
   iof.close = fmemopen_close;
 
-  return _IO_fopencookie (c, mode, iof);
+  ret = _IO_fopencookie (c, mode, iof);
+  if (__glibc_unlikely (ret == NULL))
+    {
+      /* BZ 18757 -- set EINVAL  */
+      __set_errno (EINVAL);
+      free (c);
+    }
+
+  return ret;
 }
 compat_symbol (libc, __old_fmemopen, fmemopen, GLIBC_2_2);
 #endif
diff --git a/libio/test-fmemopen.c b/libio/test-fmemopen.c
index 63ca89f..4d15b36 100644
--- a/libio/test-fmemopen.c
+++ b/libio/test-fmemopen.c
@@ -24,6 +24,28 @@  static char buffer[] = "foobar";
 #include <errno.h>
 
 static int
+do_bz18757 (void)
+{
+  char c = 0;
+  FILE *stream;
+
+  errno = 0;
+  stream = fmemopen (&c, 1, "?");
+  if (stream == NULL)
+    {
+      if (errno == EINVAL)
+        return 0;
+
+      printf ("FAIL: errno = %i, but wanted EINVAL (%i)\n", errno, EINVAL);
+      return 1;
+    }
+
+  printf ("FAIL: stream = %p, but wanted NULL\n", stream);
+  fclose (stream);
+  return 2;
+}
+
+static int
 do_test (void)
 {
   int ch;
@@ -44,7 +66,7 @@  do_test (void)
 
   fclose (stream);
 
-  return ret;
+  return ret + do_bz18757 ();
 }
 
 #define TEST_FUNCTION do_test ()