GNOME Bugzilla – Bug 427992
leak in bonobo_activation_set_activation_env_value
Last modified: 2007-12-12 13:57:58 UTC
It looks to me as if said function resizes the activation_environment._buffer and then tries to copy over the old entries before appending the new one and freeing the old buffer. But it uses Bonobo_ActivationEnvValue_copy for the copying, which duplicates the name and value of each entry. This causes the final CORBA_free (old_buffer) to leak the names and values of the old entries. Here is a patch that replaces the Bonobo_ActivationEnvValue_copy by simple struct assignment, and should fix this leak.
Created attachment 86065 [details] [review] env-leak.patch
Looks good.
Both branches or is it too risky to commit to the stable branch?
I think it's safe for both branches; I've patched my libbonobo system-wide and notice no crashes in any app, including panel, applets, nautilus, and evolution.
It should be safe enough for both branches. I'll point out though, that even with this patch, valgrind still complains about possible leaks in Bonobo_ActivationEnvValue_set(), which I don't quite understand.
Committed to both branches: 2007-04-10 Matthias Clasen <mclasen@redhat.com> * bonobo-activation/bonobo-activation-activate.c (bonobo_activation_set_activation_env_value): Don't leak old key-value pairs when copying to a larger array. (#427992)
I get this in my jhbuild setup now: ==9230== Invalid read of size 1 ==9230== at 0x4023288: strlen (mc_replace_strmem.c:246) ==9230== by 0x4EA3C97: giop_send_buffer_append_string (giop-send-buffer.c:515) ==9230== by 0x4EB235F: ORBit_marshal_value (corba-any.c:217) ==9230== by 0x4EB2012: ORBit_marshal_value (corba-any.c:175) ==9230== by 0x4EB23F1: ORBit_marshal_value (corba-any.c:237) ==9230== by 0x4EA87A7: orbit_small_marshal (orbit-small.c:353) ==9230== by 0x4EA9C19: ORBit_small_invoke_stub (orbit-small.c:646) ==9230== by 0x4EA9E5D: ORBit_small_invoke_stub_n (orbit-small.c:575) ==9230== by 0x4EB6761: ORBit_c_stub_invoke (poa.c:2643) ==9230== by 0x4AA8449: Bonobo_ActivationContext_activateMatchingFull (Bonobo_ActivationContext-stubs.c:87) ==9230== by 0x4AAAD48: bonobo_activation_activate (bonobo-activation-activate.c:380) ==9230== by 0x4AAAF77: bonobo_activation_activate_from_id (bonobo-activation-activate.c:437) ==9230== by 0x807B309: nautilus_application_startup (nautilus-application.c:598) ==9230== by 0x808C2FD: main (nautilus-main.c:533) ==9230== Address 0x58B02D1 is 1 bytes inside a block of size 9 free'd ==9230== at 0x40220FF: free (vg_replace_malloc.c:233) ==9230== by 0x4F75F40: g_free (gmem.c:187) ==9230== by 0x4EADAB7: ORBit_free_T (allocators.c:168) ==9230== by 0x4EADD65: ORBit_freekids_via_TypeCode_T (allocators.c:93) ==9230== by 0x4EADDF7: ORBit_freekids_via_TypeCode_T (allocators.c:66) ==9230== by 0x4EADAE4: ORBit_free_T (allocators.c:202) ==9230== by 0x4EADB70: ORBit_free (allocators.c:218) ==9230== by 0x4EADBDC: CORBA_free (allocators.c:143) ==9230== by 0x4AAA35F: bonobo_activation_set_activation_env_value (bonobo-activation-activate.c:825) ==9230== by 0x808BEF4: main (nautilus-main.c:467) ==9230== Invalid read of size 1 ==9230== at 0x4023293: strlen (mc_replace_strmem.c:246) ==9230== by 0x4EA3C97: giop_send_buffer_append_string (giop-send-buffer.c:515) ==9230== by 0x4EB235F: ORBit_marshal_value (corba-any.c:217) ==9230== by 0x4EB2012: ORBit_marshal_value (corba-any.c:175) ==9230== by 0x4EB23F1: ORBit_marshal_value (corba-any.c:237) ==9230== by 0x4EA87A7: orbit_small_marshal (orbit-small.c:353) ==9230== by 0x4EA9C19: ORBit_small_invoke_stub (orbit-small.c:646) ==9230== by 0x4EA9E5D: ORBit_small_invoke_stub_n (orbit-small.c:575) ==9230== by 0x4EB6761: ORBit_c_stub_invoke (poa.c:2643) ==9230== by 0x4AA8449: Bonobo_ActivationContext_activateMatchingFull (Bonobo_ActivationContext-stubs.c:87) ==9230== by 0x4AAAD48: bonobo_activation_activate (bonobo-activation-activate.c:380) ==9230== by 0x4AAAF77: bonobo_activation_activate_from_id (bonobo-activation-activate.c:437) ==9230== by 0x807B309: nautilus_application_startup (nautilus-application.c:598) ==9230== by 0x808C2FD: main (nautilus-main.c:533) ==9230== Address 0x58B02D2 is 2 bytes inside a block of size 9 free'd ==9230== at 0x40220FF: free (vg_replace_malloc.c:233) ==9230== by 0x4F75F40: g_free (gmem.c:187) ==9230== by 0x4EADAB7: ORBit_free_T (allocators.c:168) ==9230== by 0x4EADD65: ORBit_freekids_via_TypeCode_T (allocators.c:93) ==9230== by 0x4EADDF7: ORBit_freekids_via_TypeCode_T (allocators.c:66) ==9230== by 0x4EADAE4: ORBit_free_T (allocators.c:202) ==9230== by 0x4EADB70: ORBit_free (allocators.c:218) ==9230== by 0x4EADBDC: CORBA_free (allocators.c:143) ==9230== by 0x4AAA35F: bonobo_activation_set_activation_env_value (bonobo-activation-activate.c:825) ==9230== by 0x808BEF4: main (nautilus-main.c:467)
I guess that function needs to be rewritten almost from scratch to take advantage of the new-ish ORBit_sequence_* utility functions.
In the meantime I guess leaking is preferred to invalid reads and potential crashes?
I guess you're right. Can you revert the patch? Soon (before 2.19.1 I hope) I'll try to do a proper fix, for inclusion in the development branch.
OK, fixed in both branches.
What was the solution for this?
I think that this patch (http://svn.gnome.org/viewcvs/libbonobo/branches/gnome-2-18/bonobo-activation/bonobo-activation-activate.c?r1=3364&r2=3370) have something related with this bug. This patch make the magnifier work incorrectly. You can have more informations going to bug #468374 The basic problem is that we fetch the DISPLAY env variable and after the application of this patch we are getting :0 instead of :0.0
I just added a unit test to check the patch works as expected, and I can't find any problem. It's possible the API was badly implemented before and the patch fixed it, thereby changing behaviour. Bottom line, I don't know exactly what to fix, so...
I guess that libbonobo in some way manipulate the DISPLAY variable, but I don't know exactly where and how. I'm trying to find where it's, but if someone more close to the code can help, it will be of great value. The comments in http://bugzilla.gnome.org/show_bug.cgi?id=468374#c27 give great indications that this is happening in the scope of the patch that I indicate in my last comment, since without the patch we get the proper value for DISPLAY.
> I guess that libbonobo in some way manipulate the DISPLAY variable, but I don't > know exactly where and how. Is there any chance that the at-spi is the one doing the manipulation?
:0 vs :0.0 is quite normal. Unfortunately both are synonyms for the same display - and b-a-s can't tell, it just does a string compare. There is some way to normalise the display name (with a gdk call) before registering with b-a-s; it is altogether possible that the magnifier does something 'clever' in this regard wrt. being a multi-display sort of application. Beyond that I can't help :-)
i think i have discovered what is bogus about this patch. i've been tracking down for weeks the cause of why i can't run two gnome sessions concurrently anymore (i use freenx and vnc as well as a local desktop). this patch is the cause, and i have pinpointed the reason. if you check the diff, referenced in comment #13, you see that the old code actually initialized the VALUE of the getenv_values[i] objects. during the code cleanup, the inititalization of the values with getenv was removed. by reintroducting the getenv, my problems are fixed. i will attach a patch against 2.20.1
Created attachment 100796 [details] [review] reintroduce the getenv to initialize the activation environment tested to fix my problem running concurrent gnome sessions for same user
(In reply to comment #18) > during the code cleanup, the inititalization of the values with getenv was > removed. by reintroducting the getenv, my problems are fixed. i will attach a > patch against 2.20.1 Yes, sorry, this was my fault. With all the refactoring I missed that part. Patch looks good to me.
Released libbonobo 2.20.2 with the fix.