Bug 21259

Indeed.

Created attachment 86707 [details] [review]
[PATCH] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist

Let's fix it.

Comment on attachment 86707 [details] [review]
[PATCH] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist

Review of attachment 86707 [details] [review]:
-----------------------------------------------------------------

The bug here is that if you do

_dbus_message_iter_get_args_valist (&iter, &error,
    DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &as,
    DBUS_TYPE_UNIX_FD, &fd,
    DBUS_TYPE_UINT32, &u,
    DBUS_TYPE_INVALID)

then on failure to parse 'u', 'fd' and 'as' already contain copies which aren't freed.

Your patch fixes that simple case, but it does not fix this:

_dbus_message_iter_get_args_valist (&iter, &error,
    DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &as1,
    DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &as2,
    DBUS_TYPE_UNIX_FD, &fd1,
    DBUS_TYPE_UNIX_FD, &fd2,
    DBUS_TYPE_UINT32, &u,
    DBUS_TYPE_INVALID)

You'd close fd2 and free as2, but not fd1 or as1.

I think the only way to fix this fully would be to copy the var_args before iteration. On failure, we'd have a second loop over the copy, just to free/close them, stopping at the argument we failed to parse (you could use the value of i to track that). It should also set as1 = NULL, as2 = NULL, fd1 = -1, fd2 = -1 etc. to make callers more deterministic.

I can easily imagine existing callers working around this bug by closing/freeing the arguments themselves on failure - which they're not meant to have to do, but it would currently work. Setting the output variables to dummy values would either mean the caller's attempts to free them didn't do anything (if they used a NULL-tolerant free-function), or failing that, at least give them a better backtrace.

I think this is too long-standing and subtle to fix in 1.6. 1.7 would be OK.

(In reply to comment #3)
> Comment on attachment 86707 [details] [review] [review]
> [PATCH] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist
> 
> Review of attachment 86707 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> The bug here is that if you do
> 
> _dbus_message_iter_get_args_valist (&iter, &error,
>     DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &as,
>     DBUS_TYPE_UNIX_FD, &fd,
>     DBUS_TYPE_UINT32, &u,
>     DBUS_TYPE_INVALID)
> 
> then on failure to parse 'u', 'fd' and 'as' already contain copies which
> aren't freed.
> 
> Your patch fixes that simple case, but it does not fix this:
> 
> _dbus_message_iter_get_args_valist (&iter, &error,
>     DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &as1,
>     DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &as2,
>     DBUS_TYPE_UNIX_FD, &fd1,
>     DBUS_TYPE_UNIX_FD, &fd2,
>     DBUS_TYPE_UINT32, &u,
>     DBUS_TYPE_INVALID)
> 
> You'd close fd2 and free as2, but not fd1 or as1.

Good point!

> 
> I think the only way to fix this fully would be to copy the var_args before
> iteration. On failure, we'd have a second loop over the copy, just to
> free/close them, stopping at the argument we failed to parse (you could use
> the value of i to track that). It should also set as1 = NULL, as2 = NULL,
> fd1 = -1, fd2 = -1 etc. to make callers more deterministic.

Yes, I'll create a patch to do that. Another way is to save all duped memory to a structure, but need dynamic expand, not good.

> 
> I can easily imagine existing callers working around this bug by
> closing/freeing the arguments themselves on failure - which they're not
> meant to have to do, but it would currently work. Setting the output
> variables to dummy values would either mean the caller's attempts to free
> them didn't do anything (if they used a NULL-tolerant free-function), or
> failing that, at least give them a better backtrace.
> 
> I think this is too long-standing and subtle to fix in 1.6. 1.7 would be OK.

Created attachment 87428 [details] [review]
[PATCH v2] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist

copy va_list first and then do another iteration over it to free may leaked memory and unix fds.

Created attachment 87430 [details]
test client based on client.c from fd.o #13305

Created attachment 87431 [details]
test server based on server.c from fd.o #13305

Created attachment 87434 [details] [review]
[PATCH v3 1/2] Correctly set number of arguments already handled

Created attachment 87435 [details] [review]
[PATCH v3 2/2] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist

Created attachment 87436 [details]
test client based on client.c from fd.o #13305

Created attachment 87437 [details]
test server based on server.c from fd.o #13305

Created attachment 87438 [details]
output without this fix

Created attachment 87439 [details]
output with this fix

There are finally two patches, two test files (yeah, very ugly, just to do test temporarily, :-)) most based on files from #bug13305 and output we got without/with this fix applied.

Another off topic question, should we set fd to '-1' in _dbus_close() and array of string pointer to NULL dbus_free_string_array()?

Comment on attachment 87435 [details] [review]
[PATCH v3 2/2] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist

Review of attachment 87435 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-message.c
@@ +811,5 @@
> +  /* copy var_args first, then we can do another iteration over it to
> +   * free memory and close unix fds if parse failed at some point.
> +   */
> +  va_copy (copy_args, var_args);
> +  va_end (var_args);

I don't see how this va_end makes sense? The function uses var_args after this point, so it's incorrect to have "freed" it.

@@ +998,4 @@
>          }
>      }
>  
> +  return TRUE;

This is wrong if va_copy allocates memory. va_copy(3) says "Each invocation of va_copy() must be matched by a corresponding invocation of va_end() in the same function".

va_copy(3) also says "On some systems, va_end contains a closing '}' matching a '{' in va_start". I think that would necessarily imply that va_copy() contains a '{' on such systems.

I think this structure would work:

  va_copy (copy_args, var_args);

  while (spec_type != DBUS_TYPE_INVALID)
    {
      ... use var_args ...
    }

  retval = TRUE;

out:
  if (!retval)
    {
      ... use copy_args ...
    }

  va_end (copy_args);
  return retval;

@@ +1014,5 @@
> +        {
> +#ifdef HAVE_UNIX_FD_PASSING
> +          int *pfd;
> +
> +          pfd = va_arg (copy_args, int*);

nitpicking: int *, with the space

(I like pointer types in newly-added lines of code to have the form of whitespace that suggests the actual C precedence: see <http://telepathy.freedesktop.org/wiki/Style/#misc>.)

@@ +1017,5 @@
> +
> +          pfd = va_arg (copy_args, int*);
> +          _dbus_assert(pfd);
> +	  if (*pfd > 0)
> +            _dbus_close (*pfd, NULL);

I'd prefer

if (*pfd > 0)
  {
    _dbus_close (*pfd, NULL);
    *pfd = -1;
  }

so that if a caller is trying to work around this leak, they'll just close(-1) and fail, rather than trying to close what has potentially become another thread's valid fd.

@@ +1023,5 @@
> +        }
> +      else if (dbus_type_is_basic (spec_type))
> +        {
> +          /* move the index forward */
> +          va_arg (copy_args, DBusBasicValue*);

DBusBasicValue *

@@ +1034,5 @@
> +          if (dbus_type_is_fixed (spec_element_type))
> +            {
> +              /* move the index forward */
> +              va_arg (copy_args, const DBusBasicValue**);
> +              va_arg (copy_args, int*);

const DBusBasicValue **, int *

@@ +1042,5 @@
> +              char ***str_array_p;
> +
> +              str_array_p = va_arg (copy_args, char***);
> +	      /* move the index forward */
> +	      va_arg (copy_args, int*);

char ***, int *

@@ +1044,5 @@
> +              str_array_p = va_arg (copy_args, char***);
> +	      /* move the index forward */
> +	      va_arg (copy_args, int*);
> +              _dbus_assert (str_array_p != NULL);
> +	      dbus_free_string_array (*str_array_p);

add: *str_array_p = NULL

(same reason as for the fds)

Please add an actual automated regression test. You shouldn't need to actually send any messages: you should be able to just construct a DBusMessage, do stuff with it, and free it. If you add the testing to dbus-message-util.c, you can use the existing memory- and fd-leak checking code.

Something like this pseudocode, perhaps:

* Construct a message of signature 'uashuashu' or something (where each 'h' is a copy of v_UNIX_FD or something, and each 'as' is a copy of v_ARRAY_STRING)

* Parse it with ..._get_args() using its correct signature, and verify that everything is OK. Close the two fds and free the two string arrays. Expect no error and no fd leaks.

* Parse it with ..._get_args() as if expecting signatures "", "u", "uas", "uash", "uashu", "uashuas", "uashuashu". In each case, expect an error and free it. Again, expect no fd leaks.

* Free the message. Expect no memory leaks (check_memleaks()).

You can test for fd leaks like this:

initial_fds = _dbus_check_fdleaks_enter ();
... do something that shouldn't leak any fds ...
_dbus_check_fdleaks_leave (initial_fds);

(In reply to comment #14)
> Another off topic question, should we set fd to '-1' in _dbus_close() and
> array of string pointer to NULL dbus_free_string_array()?

*In* those functions, it wouldn't be useful to do that. (Think about where the fd/pointer variable is situated - either in a register or on the stack - and why it wouldn't have a side-effect on the rest of the program.)

*When calling* those functions, yes, it's sometimes useful to do that.

Some libraries have this pattern (I first saw it in CPython, added it to Telepathy, and it's now used in GLib):

    Thing *thing;

    thing = thing_new ();
    ...
    thing_clear (&thing);    // similar to { thing_free (thing); thing = NULL; }

which can also be used for fds (using -1 instead of NULL as the "clearing" value). However, to be useful, thing_clear() needs its argument to have an extra level of "pointerness" beyond what you'd expect a free-function to have - a (Thing **) instead of the usual (Thing *), or for a fd, an (int *) instead of an int.

(In reply to comment #15)
> @@ +1042,5 @@
> > +              char ***str_array_p;
> > +
> > +              str_array_p = va_arg (copy_args, char***);
> > +	      /* move the index forward */
> > +	      va_arg (copy_args, int*);
...
> > +              str_array_p = va_arg (copy_args, char***);
> > +	      /* move the index forward */
> > +	      va_arg (copy_args, int*);
> > +              _dbus_assert (str_array_p != NULL);
> > +	      dbus_free_string_array (*str_array_p);

I didn't see this when reviewing in Splinter, presumably because it normalizes whitespace or something, but something seems to have gone wrong with the indentation here.

Please indent all newly-added lines of code with spaces, even if they were adapted from a copy of existing code that mixed spaces and tabs. You might find one of these useful:

* git show | sed -e 's/    /»·······/'
  (where the gap between the first two slashes is a literal tab
  character, [Ctrl+V][Tab] in most shells)

* an editor that makes odd whitespace visible, e.g. in gvim or vim in
  a UTF-8 terminal, I use:

  set list listchars=tab:»·,trail:¯,nbsp:¤

(In reply to comment #15)
> Comment on attachment 87435 [details] [review] [review]
> [PATCH v3 2/2] Fix memory or unix fd may leak in 
> dbus_message_iter_get_args_valist
> 
> Review of attachment 87435 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-message.c
> @@ +811,5 @@
> > +  /* copy var_args first, then we can do another iteration over it to
> > +   * free memory and close unix fds if parse failed at some point.
> > +   */
> > +  va_copy (copy_args, var_args);
> > +  va_end (var_args);
> 
> I don't see how this va_end makes sense? The function uses var_args after
> this point, so it's incorrect to have "freed" it.

definitely, I was mixed with dst and scr of va_copy in va_copy(3).

> 
> @@ +998,4 @@
> >          }
> >      }
> >  
> > +  return TRUE;
> 
> This is wrong if va_copy allocates memory. va_copy(3) says "Each invocation
> of va_copy() must be matched by a corresponding invocation of va_end() in
> the same function".
> 
> va_copy(3) also says "On some systems, va_end contains a closing '}'
> matching a '{' in va_start". I think that would necessarily imply that
> va_copy() contains a '{' on such systems.
> 
> I think this structure would work:
> 
>   va_copy (copy_args, var_args);
> 
>   while (spec_type != DBUS_TYPE_INVALID)
>     {
>       ... use var_args ...
>     }
> 
>   retval = TRUE;
> 
> out:
>   if (!retval)
>     {
>       ... use copy_args ...
>     }
> 
>   va_end (copy_args);
>   return retval;
> 
> @@ +1014,5 @@
> > +        {
> > +#ifdef HAVE_UNIX_FD_PASSING
> > +          int *pfd;
> > +
> > +          pfd = va_arg (copy_args, int*);
> 
> nitpicking: int *, with the space
> 
> (I like pointer types in newly-added lines of code to have the form of
> whitespace that suggests the actual C precedence: see
> <http://telepathy.freedesktop.org/wiki/Style/#misc>.)
> 
> @@ +1017,5 @@
> > +
> > +          pfd = va_arg (copy_args, int*);
> > +          _dbus_assert(pfd);
> > +	  if (*pfd > 0)
> > +            _dbus_close (*pfd, NULL);
> 
> I'd prefer
> 
> if (*pfd > 0)
>   {
>     _dbus_close (*pfd, NULL);
>     *pfd = -1;
>   }
> 
> so that if a caller is trying to work around this leak, they'll just
> close(-1) and fail, rather than trying to close what has potentially become
> another thread's valid fd.
> 
> @@ +1023,5 @@
> > +        }
> > +      else if (dbus_type_is_basic (spec_type))
> > +        {
> > +          /* move the index forward */
> > +          va_arg (copy_args, DBusBasicValue*);
> 
> DBusBasicValue *
> 
> @@ +1034,5 @@
> > +          if (dbus_type_is_fixed (spec_element_type))
> > +            {
> > +              /* move the index forward */
> > +              va_arg (copy_args, const DBusBasicValue**);
> > +              va_arg (copy_args, int*);
> 
> const DBusBasicValue **, int *
> 
> @@ +1042,5 @@
> > +              char ***str_array_p;
> > +
> > +              str_array_p = va_arg (copy_args, char***);
> > +	      /* move the index forward */
> > +	      va_arg (copy_args, int*);
> 
> char ***, int *
> 
> @@ +1044,5 @@
> > +              str_array_p = va_arg (copy_args, char***);
> > +	      /* move the index forward */
> > +	      va_arg (copy_args, int*);
> > +              _dbus_assert (str_array_p != NULL);
> > +	      dbus_free_string_array (*str_array_p);
> 
> add: *str_array_p = NULL
> 
> (same reason as for the fds)

(In reply to comment #18)
> (In reply to comment #15)
> > @@ +1042,5 @@
> > > +              char ***str_array_p;
> > > +
> > > +              str_array_p = va_arg (copy_args, char***);
> > > +	      /* move the index forward */
> > > +	      va_arg (copy_args, int*);
> ...
> > > +              str_array_p = va_arg (copy_args, char***);
> > > +	      /* move the index forward */
> > > +	      va_arg (copy_args, int*);
> > > +              _dbus_assert (str_array_p != NULL);
> > > +	      dbus_free_string_array (*str_array_p);
> 
> I didn't see this when reviewing in Splinter, presumably because it
> normalizes whitespace or something, but something seems to have gone wrong
> with the indentation here.
> 

yes, I copied some lines from the first loop and there were two \t left, so I'll fix all of them in this patch.

> Please indent all newly-added lines of code with spaces, even if they were
> adapted from a copy of existing code that mixed spaces and tabs. You might
> find one of these useful:
> 
> * git show | sed -e 's/    /»·······/'
>   (where the gap between the first two slashes is a literal tab
>   character, [Ctrl+V][Tab] in most shells)
> 
> * an editor that makes odd whitespace visible, e.g. in gvim or vim in
>   a UTF-8 terminal, I use:
> 
>   set list listchars=tab:»·,trail:¯,nbsp:¤

thanks for tips.

(In reply to comment #16)
> Please add an actual automated regression test. You shouldn't need to
> actually send any messages: you should be able to just construct a
> DBusMessage, do stuff with it, and free it. If you add the testing to
> dbus-message-util.c, you can use the existing memory- and fd-leak checking
> code.
> 
> Something like this pseudocode, perhaps:
> 
> * Construct a message of signature 'uashuashu' or something (where each 'h'
> is a copy of v_UNIX_FD or something, and each 'as' is a copy of
> v_ARRAY_STRING)
> 
> * Parse it with ..._get_args() using its correct signature, and verify that
> everything is OK. Close the two fds and free the two string arrays. Expect
> no error and no fd leaks.
> 
> * Parse it with ..._get_args() as if expecting signatures "", "u", "uas",

I found that currently _dbus_message_iter_get_args_valist() return TRUE if the first argument is DBUS_TYPE_INVALID.

Should we change it to return FALSE? Seems a imcomptable change if so.

> "uash", "uashu", "uashuas", "uashuashu". In each case, expect an error and
> free it. Again, expect no fd leaks.
> 
> * Free the message. Expect no memory leaks (check_memleaks()).
> 
> You can test for fd leaks like this:
> 
> initial_fds = _dbus_check_fdleaks_enter ();
> ... do something that shouldn't leak any fds ...
> _dbus_check_fdleaks_leave (initial_fds);

(In reply to comment #16)
> Please add an actual automated regression test. You shouldn't need to
> actually send any messages: you should be able to just construct a
> DBusMessage, do stuff with it, and free it. If you add the testing to
> dbus-message-util.c, you can use the existing memory- and fd-leak checking
> code.
> 
> Something like this pseudocode, perhaps:
> 
> * Construct a message of signature 'uashuashu' or something (where each 'h'
> is a copy of v_UNIX_FD or something, and each 'as' is a copy of
> v_ARRAY_STRING)
> 
> * Parse it with ..._get_args() using its correct signature, and verify that
> everything is OK. Close the two fds and free the two string arrays. Expect
> no error and no fd leaks.
> 
> * Parse it with ..._get_args() as if expecting signatures "", "u", "uas",
> "uash", "uashu", "uashuas", "uashuashu". In each case, expect an error and
> free it. Again, expect no fd leaks.

All the above cases are expected not fail as below documents in dbus_message_get_args()

 * If the requested arguments are not present, or do not have the
 * requested types, then an error will be set.
 *
 * If more arguments than requested are present, the requested
 * arguments are returned and the extra arguments are ignored.

So I'll fail them in another pattern.

> 
> * Free the message. Expect no memory leaks (check_memleaks()).
> 
> You can test for fd leaks like this:
> 
> initial_fds = _dbus_check_fdleaks_enter ();
> ... do something that shouldn't leak any fds ...
> _dbus_check_fdleaks_leave (initial_fds);

Created attachment 88263 [details] [review]
[PATCH v4 1/3] Correctly set number of arguments already handled

only did rebase

Created attachment 88264 [details] [review]
[PATCH v4 2/3] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist

addressed Simon's comments, in addition a typo str_array_p = NULL --> *str_array_p = NULL

Created attachment 88265 [details] [review]
[PATCH v4 3/3] Test: add test cases for message parsing

test cases for message parsing, for two kinds of testing

1. arguments present but not requested are ignored

2. arguments parse failed should not leak memory or unix fd.

(In reply to comment #22)
> > * Parse it with ..._get_args() as if expecting signatures "", "u", "uas",
> > "uash", "uashu", "uashuas", "uashuashu". In each case, expect an error and
> > free it. Again, expect no fd leaks.
> 
> All the above cases are expected not fail as below documents in
> dbus_message_get_args()

OK, my mistake - I'd forgotten that corner of the API. In that case, please expect those to succeed, without leaking.

Comment on attachment 88264 [details] [review]
[PATCH v4 2/3] Fix memory or unix fd may leak in  dbus_message_iter_get_args_valist

Review of attachment 88264 [details] [review]:
-----------------------------------------------------------------

Makes sense. I'll fix the one error I noted when I apply it, if you don't get there first.

::: dbus/dbus-message.c
@@ +1018,5 @@
> +              int *pfd;
> +
> +              pfd = va_arg (copy_args, int *);
> +              _dbus_assert(pfd);
> +              if (*pfd > 0)

>= 0, strictly speaking. 0 is a valid fd too (although it's stdin, so it will nearly always be open already).

Comment on attachment 88265 [details] [review]
[PATCH v4 3/3] Test: add test cases for message parsing

Review of attachment 88265 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-message-util.c
@@ +991,5 @@
>      _dbus_assert_not_reached ("Didn't reach end of arguments");
>  }
>  
> +static void
> +verify_test_message_args_ignored (DBusMessage *message)

I'd prefer this test to check for fd-leaks too. I'll add that when I commit it if necessary.

(In reply to comment #27)
> Comment on attachment 88264 [details] [review] [review]
> [PATCH v4 2/3] Fix memory or unix fd may leak in 
> dbus_message_iter_get_args_valist
> 
> Review of attachment 88264 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Makes sense. I'll fix the one error I noted when I apply it, if you don't
> get there first.

Sure, thank you.

> 
> ::: dbus/dbus-message.c
> @@ +1018,5 @@
> > +              int *pfd;
> > +
> > +              pfd = va_arg (copy_args, int *);
> > +              _dbus_assert(pfd);
> > +              if (*pfd > 0)
> 
> >= 0, strictly speaking. 0 is a valid fd too (although it's stdin, so it will nearly always be open already).

Yeah, make sense.

(In reply to comment #28)
> Comment on attachment 88265 [details] [review] [review]
> [PATCH v4 3/3] Test: add test cases for message parsing
> 
> Review of attachment 88265 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-message-util.c
> @@ +991,5 @@
> >      _dbus_assert_not_reached ("Didn't reach end of arguments");
> >  }
> >  
> > +static void
> > +verify_test_message_args_ignored (DBusMessage *message)
> 
> I'd prefer this test to check for fd-leaks too. I'll add that when I commit
> it if necessary.

Hmm, It was there but removed before I upload this patch since I think it should not happen there. :-), anyway, your decision.

BTW, did you noticed the first patch? Without it, the second one will take incorrect "i" as a loop stop condition.

Created attachment 88293 [details]
[PATCH v5] Fix memory or unix fd may leak in dbus_message_iter_get_args_valist

> > +              if (*pfd > 0)
> 
> >= 0, strictly speaking. 0 is a valid fd too (although it's stdin, so it will nearly always be open already).

Fixed.

already merged into master, for 1.7.8.