Bug 10834

Summary:	dbus_g_proxy_end_call_internal() frees random memory on error
Product:	dbus	Reporter:	Peter Kjellerstedt <pkj>
Component:	GLib	Assignee:	Rob Taylor <rob.taylor>
Status:	RESOLVED FIXED	QA Contact:	John (J5) Palmieri <johnp>
Severity:	normal
Priority:	medium	CC:	mikhail.zabaluev, walters
Version:	unspecified
Hardware:	All
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	Correct argument unwinding in dbus_g_proxy_end_call_internal() Correct argument unwinding in dbus_g_proxy_end_call_internal()

Description Peter Kjellerstedt 2007-05-02 07:22:23 UTC

There are a couple of errors in dbus_g_proxy_end_call_internal() related to freeing memory after an error condition. 

First of all the va_list args_unwind is never initialized. I looked back at earlier versions of this file and found that it was initialized when this code was first added, but the initialization seems to have been lost in a refactoring of the code.

Second, va_end() is called on args eventhough this is done in all calling functions (where it belongs AFAIK).

Third, the clean up code at the end of dbus_g_proxy_end_call_internal() frees the first n_retvals_processed arguments, regardless of the fact that every other argument is the type of the next argument...

Fourth, it tries to free all arguments regardless of type.

And fifth, it should free what the arguments point at rather than the arguments themselves.

Comment 1 Peter Kjellerstedt 2007-05-02 07:26:29 UTC

Created attachment 9838 [details] [review]
Correct argument unwinding in dbus_g_proxy_end_call_internal()

This patch should fix all the problems I have reported. Possibly, the switch statement freeing the arguments depending on their type should be in some helper function in dbus-gvalue-utils.c.

Comment 2 Peter Kjellerstedt 2007-05-02 07:32:52 UTC

Created attachment 9839 [details] [review]
Correct argument unwinding in dbus_g_proxy_end_call_internal()

Small update to the patch for better coding style conformance.

Comment 3 Peter Kjellerstedt 2007-05-02 07:43:07 UTC

I forgot to mention that I have not tried the code for freeing G_TYPE_BOXED and G_TYPE_OBJECT as we do not use either in our code, but I hope I got it right.

Comment 4 Ross Burton 2007-06-22 06:56:25 UTC

Rob, this looks good to me, want to have a look?

Comment 5 Colin Walters 2008-05-27 13:53:18 UTC

This patch had some flaws:

o It removed the call to va_end (args)
o We want to handle all argument types, not just the few in
  the switch.  So we use the internal _dbus_gvalue_take 
  which uses our GValue machinery to handle argument types
  in a generic way

I fixed those issues and also some others, see the log:

commit 43db9baa4cd0921d2ee830185ab46b4646b4e73b
Author: Colin Walters <walters@verbum.org>
Date:   Tue May 27 16:49:26 2008 -0400

    Bug 10834: Fix error handling path for dbus_g_proxy_end_call_internal
    
    This patch was based initial work by Peter Kjellerstedt.
    
    This patch made obvious the need to correctly handle type
    mismatches in demarshal_basic, similarly to what the other
    demarshalers are doing.
    
    Also add some tests for error handling.

Comment 6 Colin Walters 2008-05-27 19:42:30 UTC

*** Bug 9063 has been marked as a duplicate of this bug. ***

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.