diff mbox

fopencookie: Mangle function pointers stored on the heap [BZ #20222]

Message ID 20160610104919.479B340176C1C@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer June 10, 2016, 10:49 a.m. UTC 
2016-06-10  Florian Weimer  <fweimer@redhat.com>

	[BZ #20222]
	* libio/iofopncook.c (_IO_cookie_read): Demangle callback pointer.
	(_IO_cookie_write): Likewise.
	(_IO_cookie_seek): Likewise.
	(_IO_cookie_close): Likewise.
	(_IO_old_cookie_seek): Likewise.
	(set_callbacks): New function.
	(_IO_cookie_init): Call set_callbacks to copy callbacks.

Comments

Carlos O'Donell June 10, 2016, 3:55 p.m. UTC | #1 
On 06/10/2016 06:49 AM, Florian Weimer wrote:
> 2016-06-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #20222]
> 	* libio/iofopncook.c (_IO_cookie_read): Demangle callback pointer.
> 	(_IO_cookie_write): Likewise.
> 	(_IO_cookie_seek): Likewise.
> 	(_IO_cookie_close): Likewise.
> 	(_IO_old_cookie_seek): Likewise.
> 	(set_callbacks): New function.
> 	(_IO_cookie_init): Call set_callbacks to copy callbacks.

A couple of questions...
 
> diff --git a/libio/iofopncook.c b/libio/iofopncook.c
> index 9eda7c1..487297c 100644
> --- a/libio/iofopncook.c
> +++ b/libio/iofopncook.c
> @@ -43,25 +43,29 @@ static _IO_ssize_t
>  _IO_cookie_read (_IO_FILE *fp, void *buf, _IO_ssize_t size)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_read_function_t *read_cb = cfile->__io_functions.read;
> +  PTR_DEMANGLE (read_cb);
>  
> -  if (cfile->__io_functions.read == NULL)
> +  if (read_cb == NULL)
>      return -1;
>  
> -  return cfile->__io_functions.read (cfile->__cookie, buf, size);
> +  return read_cb (cfile->__cookie, buf, size);
>  }
>  
>  static _IO_ssize_t
>  _IO_cookie_write (_IO_FILE *fp, const void *buf, _IO_ssize_t size)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_write_function_t *write_cb = cfile->__io_functions.write;
> +  PTR_DEMANGLE (write_cb);
>  
> -  if (cfile->__io_functions.write == NULL)
> +  if (write_cb == NULL)
>      {
>        fp->_flags |= _IO_ERR_SEEN;
>        return 0;
>      }
>  
> -  _IO_ssize_t n = cfile->__io_functions.write (cfile->__cookie, buf, size);
> +  _IO_ssize_t n = write_cb (cfile->__cookie, buf, size);
>    if (n < size)
>      fp->_flags |= _IO_ERR_SEEN;
>  
> @@ -72,9 +76,11 @@ static _IO_off64_t
>  _IO_cookie_seek (_IO_FILE *fp, _IO_off64_t offset, int dir)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_seek_function_t *seek_cb = cfile->__io_functions.seek;
> +  PTR_DEMANGLE (seek_cb);
>  
> -  return ((cfile->__io_functions.seek == NULL
> -	   || (cfile->__io_functions.seek (cfile->__cookie, &offset, dir)
> +  return ((seek_cb == NULL
> +	   || (seek_cb (cfile->__cookie, &offset, dir)
>  	       == -1)
>  	   || offset == (_IO_off64_t) -1)
>  	  ? _IO_pos_BAD : offset);
> @@ -84,11 +90,13 @@ static int
>  _IO_cookie_close (_IO_FILE *fp)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_close_function_t *close_cb = cfile->__io_functions.close;
> +  PTR_DEMANGLE (close_cb);
>  
> -  if (cfile->__io_functions.close == NULL)
> +  if (close_cb == NULL)
>      return 0;
>  
> -  return cfile->__io_functions.close (cfile->__cookie);
> +  return close_cb (cfile->__cookie);
>  }
>  
>  
> @@ -126,6 +134,19 @@ static const struct _IO_jump_t _IO_cookie_jumps = {
>  };
>  
>  
> +/* Copy the callbacks from SOURCE to *TARGET, with pointer
> +   mangling.  */
> +static void
> +set_callbacks (_IO_cookie_io_functions_t *target,
> +	       _IO_cookie_io_functions_t source)
> +{
> +  PTR_MANGLE (source.read);
> +  PTR_MANGLE (source.write);
> +  PTR_MANGLE (source.seek);
> +  PTR_MANGLE (source.close);

Isn't this modifying the user's copy of the callbacks
passed in during the call to fopencookie?

If it is, then that might be unexpected. I expect that we
need to copy the function pointers and then encrypt them.
Leaving the users copy untouched.

In fact I'd go as far to say that stdio-common/tst-cookie.c
should be modified to verify that 'cookie_io_functions_t fcts'
is unmodified after the call to fopencookie.

> +  *target = source;
> +}
> +
>  void
>  _IO_cookie_init (struct _IO_cookie_file *cfile, int read_write,
>  		 void *cookie, _IO_cookie_io_functions_t io_functions)
> @@ -134,7 +155,7 @@ _IO_cookie_init (struct _IO_cookie_file *cfile, int read_write,
>    _IO_JUMPS (&cfile->__fp) = &_IO_cookie_jumps;
>  
>    cfile->__cookie = cookie;
> -  cfile->__io_functions = io_functions;
> +  set_callbacks (&cfile->__io_functions, io_functions);
>  
>    _IO_file_init (&cfile->__fp);
>  
> @@ -205,14 +226,14 @@ attribute_compat_text_section
>  _IO_old_cookie_seek (_IO_FILE *fp, _IO_off64_t offset, int dir)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> -  int (*seek) (_IO_FILE *, _IO_off_t, int);
> -  int ret;
> +  int (*seek_cb) (_IO_FILE *, _IO_off_t, int)
> +    = (int (*)(_IO_FILE *, _IO_off_t, int)) cfile->__io_functions.seek;;
> +  PTR_DEMANGLE (seek_cb);
>  
> -  seek = (int (*)(_IO_FILE *, _IO_off_t, int)) cfile->__io_functions.seek;
> -  if (seek == NULL)
> +  if (seek_cb == NULL)
>      return _IO_pos_BAD;
>  
> -  ret = seek (cfile->__cookie, offset, dir);
> +  int ret = seek_cb (cfile->__cookie, offset, dir);
>  
>    return (ret == -1) ? _IO_pos_BAD : ret;
>  }
>
Florian Weimer June 10, 2016, 3:56 p.m. UTC | #2 
On 06/10/2016 05:55 PM, Carlos O'Donell wrote:

>> +/* Copy the callbacks from SOURCE to *TARGET, with pointer
>> +   mangling.  */
>> +static void
>> +set_callbacks (_IO_cookie_io_functions_t *target,
>> +	       _IO_cookie_io_functions_t source)
>> +{
>> +  PTR_MANGLE (source.read);
>> +  PTR_MANGLE (source.write);
>> +  PTR_MANGLE (source.seek);
>> +  PTR_MANGLE (source.close);
>
> Isn't this modifying the user's copy of the callbacks
> passed in during the call to fopencookie?

No, C function arguments are pass-by-value.  It's not a 
pointer-to-struct, it's the struct object itself.

Thanks,
Florian
Carlos O'Donell June 10, 2016, 5:50 p.m. UTC | #3 
On 06/10/2016 11:56 AM, Florian Weimer wrote:
> On 06/10/2016 05:55 PM, Carlos O'Donell wrote:
> 
>>> +/* Copy the callbacks from SOURCE to *TARGET, with pointer
>>> +   mangling.  */
>>> +static void
>>> +set_callbacks (_IO_cookie_io_functions_t *target,
>>> +           _IO_cookie_io_functions_t source)
>>> +{
>>> +  PTR_MANGLE (source.read);
>>> +  PTR_MANGLE (source.write);
>>> +  PTR_MANGLE (source.seek);
>>> +  PTR_MANGLE (source.close);
>>
>> Isn't this modifying the user's copy of the callbacks
>> passed in during the call to fopencookie?
> 
> No, C function arguments are pass-by-value. It's not a
> pointer-to-struct, it's the struct object itself.

Sorry yes, I misread that as pointer-to-struct.

You are absolutely right, these are just struct copies
being passed around so we don't care.

Patch looks good to me then.
diff mbox

Patch

diff --git a/libio/iofopncook.c b/libio/iofopncook.c
index 9eda7c1..487297c 100644
--- a/libio/iofopncook.c
+++ b/libio/iofopncook.c
@@ -43,25 +43,29 @@  static _IO_ssize_t
 _IO_cookie_read (_IO_FILE *fp, void *buf, _IO_ssize_t size)
 {
   struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
+  cookie_read_function_t *read_cb = cfile->__io_functions.read;
+  PTR_DEMANGLE (read_cb);
 
-  if (cfile->__io_functions.read == NULL)
+  if (read_cb == NULL)
     return -1;
 
-  return cfile->__io_functions.read (cfile->__cookie, buf, size);
+  return read_cb (cfile->__cookie, buf, size);
 }
 
 static _IO_ssize_t
 _IO_cookie_write (_IO_FILE *fp, const void *buf, _IO_ssize_t size)
 {
   struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
+  cookie_write_function_t *write_cb = cfile->__io_functions.write;
+  PTR_DEMANGLE (write_cb);
 
-  if (cfile->__io_functions.write == NULL)
+  if (write_cb == NULL)
     {
       fp->_flags |= _IO_ERR_SEEN;
       return 0;
     }
 
-  _IO_ssize_t n = cfile->__io_functions.write (cfile->__cookie, buf, size);
+  _IO_ssize_t n = write_cb (cfile->__cookie, buf, size);
   if (n < size)
     fp->_flags |= _IO_ERR_SEEN;
 
@@ -72,9 +76,11 @@  static _IO_off64_t
 _IO_cookie_seek (_IO_FILE *fp, _IO_off64_t offset, int dir)
 {
   struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
+  cookie_seek_function_t *seek_cb = cfile->__io_functions.seek;
+  PTR_DEMANGLE (seek_cb);
 
-  return ((cfile->__io_functions.seek == NULL
-	   || (cfile->__io_functions.seek (cfile->__cookie, &offset, dir)
+  return ((seek_cb == NULL
+	   || (seek_cb (cfile->__cookie, &offset, dir)
 	       == -1)
 	   || offset == (_IO_off64_t) -1)
 	  ? _IO_pos_BAD : offset);
@@ -84,11 +90,13 @@  static int
 _IO_cookie_close (_IO_FILE *fp)
 {
   struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
+  cookie_close_function_t *close_cb = cfile->__io_functions.close;
+  PTR_DEMANGLE (close_cb);
 
-  if (cfile->__io_functions.close == NULL)
+  if (close_cb == NULL)
     return 0;
 
-  return cfile->__io_functions.close (cfile->__cookie);
+  return close_cb (cfile->__cookie);
 }
 
 
@@ -126,6 +134,19 @@  static const struct _IO_jump_t _IO_cookie_jumps = {
 };
 
 
+/* Copy the callbacks from SOURCE to *TARGET, with pointer
+   mangling.  */
+static void
+set_callbacks (_IO_cookie_io_functions_t *target,
+	       _IO_cookie_io_functions_t source)
+{
+  PTR_MANGLE (source.read);
+  PTR_MANGLE (source.write);
+  PTR_MANGLE (source.seek);
+  PTR_MANGLE (source.close);
+  *target = source;
+}
+
 void
 _IO_cookie_init (struct _IO_cookie_file *cfile, int read_write,
 		 void *cookie, _IO_cookie_io_functions_t io_functions)
@@ -134,7 +155,7 @@  _IO_cookie_init (struct _IO_cookie_file *cfile, int read_write,
   _IO_JUMPS (&cfile->__fp) = &_IO_cookie_jumps;
 
   cfile->__cookie = cookie;
-  cfile->__io_functions = io_functions;
+  set_callbacks (&cfile->__io_functions, io_functions);
 
   _IO_file_init (&cfile->__fp);
 
@@ -205,14 +226,14 @@  attribute_compat_text_section
 _IO_old_cookie_seek (_IO_FILE *fp, _IO_off64_t offset, int dir)
 {
   struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
-  int (*seek) (_IO_FILE *, _IO_off_t, int);
-  int ret;
+  int (*seek_cb) (_IO_FILE *, _IO_off_t, int)
+    = (int (*)(_IO_FILE *, _IO_off_t, int)) cfile->__io_functions.seek;;
+  PTR_DEMANGLE (seek_cb);
 
-  seek = (int (*)(_IO_FILE *, _IO_off_t, int)) cfile->__io_functions.seek;
-  if (seek == NULL)
+  if (seek_cb == NULL)
     return _IO_pos_BAD;
 
-  ret = seek (cfile->__cookie, offset, dir);
+  int ret = seek_cb (cfile->__cookie, offset, dir);
 
   return (ret == -1) ? _IO_pos_BAD : ret;
 }