Consider the following code: #include <dbus/dbus.h> #include <assert.h> int main (int argc, char *arvgv[]) { DBusMessage * method_call; DBusMessageIter iter; DBusMessageIter array_iter; DBusMessageIter struct_iter; const char * str_value; method_call = dbus_message_new_method_call ("com.netsplit.Nih", "/com/netsplit/Nih", "com.netsplit.Nih.Test", "Test"); assert (method_call); dbus_message_iter_init_append (method_call, &iter); assert (dbus_message_iter_open_container (&iter, DBUS_TYPE_ARRAY, (DBUS_STRUCT_BEGIN_CHAR_AS_STRING DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_STRING_AS_STRING DBUS_STRUCT_END_CHAR_AS_STRING), &array_iter)); assert (dbus_message_iter_open_container (&array_iter, DBUS_TYPE_STRUCT, NULL, &struct_iter)); str_value = "peaches"; assert (dbus_message_iter_append_basic (&struct_iter, DBUS_TYPE_STRING, &str_value)); /* uh-oh, error, try and unwind */ assert (dbus_message_iter_close_container (&array_iter, &struct_iter)); assert (dbus_message_iter_close_container (&iter, &array_iter)); dbus_message_unref (method_call); dbus_shutdown (); return 0; } The error we try and unwind from could be a bad return from dbus_message_iter_append_basic(), a bad return from dbus_message_iter_close_container() [which "hoses" the message] or an error inside the program or bindings -- either way, we need to unwind and can't carry on. Unfortunately this isn't possible. Closing the container will assert: process 28130: Array or variant type requires that type string be written, but end_struct was written. The overall signature expected here was 'a(ss)' and we are on byte 3 of that signature. File "dbus-marshal-recursive.c" line 1726 process 28130 should not have been reached: bad type inserted somewhere inside an array or variant /lib/libdbus-1.so.3 [0x7fd078c32e62] /lib/libdbus-1.so.3 [0x7fd078c2d649] /lib/libdbus-1.so.3 [0x7fd078c1a330] /lib/libdbus-1.so.3 [0x7fd078bf2d39] /lib/libdbus-1.so.3 [0x7fd078bf381e] /lib/libdbus-1.so.3(dbus_message_iter_close_container+0x202) [0x7fd078bfb6db] ./leak [0x4008bc] /lib/libc.so.6(__libc_start_main+0xe6) [0x7fd0788795a6] ./leak [0x4006d9] zsh: abort (core dumped) ./leak Not closing the container will leak memory: ==10711== 13 bytes in 1 blocks are indirectly lost in loss record 1 of 2 ==10711== at 0x4C279E1: realloc (vg_replace_malloc.c:429) ==10711== by 0x41E2C2: dbus_realloc (dbus-memory.c:601) ==10711== by 0x41FABE: reallocate_for_length (dbus-string.c:364) ==10711== by 0x41FD28: set_length (dbus-string.c:404) ==10711== by 0x422116: _dbus_string_lengthen (dbus-string.c:872) ==10711== by 0x422985: _dbus_string_alloc_space (dbus-string.c:1005) ==10711== by 0x44613B: writer_recurse_array (dbus-marshal-recursive.c:1847) ==10711== by 0x4467A1: _dbus_type_writer_recurse_contained_len (dbus-marshal-recursive.c:2066) ==10711== by 0x44684A: _dbus_type_writer_recurse (dbus-marshal-recursive.c:2106) ==10711== by 0x40840D: dbus_message_iter_open_container (dbus-message.c:2402) ==10711== by 0x403257: main (leak.c:24) ==10711== ==10711== ==10711== 37 (24 direct, 13 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 2 ==10711== at 0x4C278AE: malloc (vg_replace_malloc.c:207) ==10711== by 0x41E05D: dbus_malloc (dbus-memory.c:471) ==10711== by 0x4076C9: _dbus_message_iter_open_signature (dbus-message.c:2113) ==10711== by 0x408374: dbus_message_iter_open_container (dbus-message.c:2393) ==10711== by 0x403257: main (leak.c:24) This is because the contained signature of the iterator isn't attached to the message, unref'ing the message will not free it.
Created attachment 26854 [details] [review] Suggested patch
One thing to figure out here is why the current memory leak/OOM tests aren't hitting this. Should we also fix say dbus_message_append_args_valist to use this function? I'd like to see a test case with the patch, probably in dbus-marshal-recursive-util.c.
Created attachment 26855 [details] [review] Patch Slight think-o in the previous patch - updated
I suspect the current OOM tests don't hit this precisely because dbus_message_append_args_valist doesn't use it? I can't see any tests for dbus_message_iter_open_container()
Created attachment 26859 [details] [review] Patch to make dbus_message_append_args_valist() abandon the array container
Created attachment 27528 [details] [review] adds an asserting test case Here's a test case that'll cause the assert (rather than the leak) to fix, we replace _close_container with the new _abandon_container functions
Looks good then, thanks!
committed and pushed
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.