GNOME Bugzilla – Bug 357879
Fix a few leaks in the greeter
Last modified: 2006-10-03 01:08:27 UTC
Valgrind found these: ==32442== 7,118 (7,114 direct, 4 indirect) bytes in 57 blocks are definitely lost in loss record 162 of 182 ==32442== at 0x4005400: malloc (vg_replace_malloc.c:149) ==32442== by 0x729785: g_malloc (gmem.c:131) ==32442== by 0x73CC68: g_strdup (gstrfuncs.c:91) ==32442== by 0x8063C1D: gdm_config_get_result (gdmconfig.c:136) ==32442== by 0x8063D67: _gdm_config_get_bool (gdmconfig.c:543) ==32442== by 0x804F9A1: main (greeter.c:715) ==32442== 259 bytes in 45 blocks are definitely lost in loss record 92 of 182 ==32442== at 0x4005400: malloc (vg_replace_malloc.c:149) ==32442== by 0x7D00805: xmlStrndup (xmlstring.c:45) ==32442== by 0x7D009AB: xmlStrdup (xmlstring.c:71) ==32442== by 0x7CAC7E5: xmlGetPropNodeValueInternal (tree.c:6106) ==32442== by 0x805A1C7: parse_items (greeter_parser.c:1585) ==32442== by 0x805AD32: greeter_parse (greeter_parser.c:1773) ==32442== by 0x805029E: main (greeter.c:1158) ==32442== 96 bytes in 2 blocks are definitely lost in loss record 77 of 182 ==32442== at 0x40054FB: realloc (vg_replace_malloc.c:306) ==32442== by 0x72964A: g_realloc (gmem.c:168) ==32442== by 0x73DD0B: g_string_maybe_expand (gstring.c:261) ==32442== by 0x73E232: g_string_insert_len (gstring.c:490) ==32442== by 0x73E585: g_string_append_len (gstring.c:530) ==32442== by 0x713CEB: g_build_path_va (gfileutils.c:1538) ==32442== by 0x713EEF: g_build_filename (gfileutils.c:1808) ==32442== by 0x804F707: get_theme_file (greeter.c:905) ==32442== by 0x8050246: main (greeter.c:1155) ==32442== 279 (8 direct, 271 indirect) bytes in 1 blocks are definitely lost in loss record 58 of 182 ==32442== at 0x400473F: calloc (vg_replace_malloc.c:279) ==32442== by 0x7296ED: g_malloc0 (gmem.c:150) ==32442== by 0x806202D: gdm_session_list_init (gdmsession.c:137) ==32442== by 0x805BD36: greeter_session_init (greeter_session.c:136) ==32442== by 0x804FF06: main (greeter.c:1071) I fixed all but the last one because I was uncertain how to go about fixing that one.
Created attachment 73458 [details] [review] patch for leaks
I've applied your patch to CVS head. Thanks for catching this. Regarding the session leak you notice... Note that the sessions are stored in a hash table and that in gdm_session_lookup is called by gdmlogin/gdmgreeter so that when the user picks a different session name, the info about the session choice can be looked up. Therefore, this hash needs to be kept around until the user no longer can change the session choice (which basically is when GDM exits). It might be good to add a function in gui/gdmsession.c that free's the hash table and then gdmlogin and gdmsession can call the function as they exit to free the array. To be honest, I'm surprised that valgrind doesn't complain about how GDM manages configuration data (daemon/gdmconfig.c, gui/gdmconfig.c). Note we do something similar here where the config data is put into hash tables that are kept around and not freed. Both the daemon and the client programs (such as gdmlogin/gdmgreeter) avoid freeing the config data. On the daemon side, this is done so that GDM doesn't have to re-read the config file when a key is requested. You have to run gdmflexiserver with the UPDATE_CONFIG command to get it to re-read a particular config data. On the slave side, this is done so that the slave doesn't have to ask the daemon for config data each time a key is requested. In both cases, if a key is requested multiple times, it is just pulled out of the hash table cache, which is faster and helps to ensure the GDM socket isn't made too busy. Like with the session hash table, we could add a cleanup function so that slave programs that use config/session hash free these hashes just before they exit. Not sure if there is any real value in freeing data where the intent is keeping it around until the process is done running, but if it makes the valgrind output easier to parse, then we can update the code to do this sort of cleanup. Note that since the memory is freed anyway when the program exits, doing such cleanup doesn't improve the memory situation much. It's probably faster for the memory to be freed in bulk when the process exits than it is to free the memory item-by-item. Odd that valgrind didn't complain about the gdmconfig hashes since they also leak. I'll leave this bug open in case you want to add such cleanup logic. If you think this isn't important, you can close.
Sounds like it's too much work for just a little gain. I did manage to find a few more leaks in gdmconfig.c though :-) Attaching a patch, but please look it over to make sure I haven't broken anything.
Created attachment 73514 [details] [review] more leak fixes
Your patch was hard to apply because it wasn't based on CVS head, but I think I updated the code as your patch recommends. Could you please verify? Marking as fixed. Re-open if you find more issues.
Just backported to 2.16. This change did not make 2.16.1, but will make 2.16.2.