Fix memory leaks - Githubissues

mate-desktop / mate-power-manager

Power management tool for the MATE desktop

https://mate-desktop.org

GNU General Public License v2.0

59 stars 51 forks source link

Fix memory leaks #367

Closed rbuj closed 2 years ago

mbkma commented 2 years ago

In the first commit there is not only a memory leaked fixed. Can you update the commit message describing what it does please?

rbuj commented 2 years ago

The local and global variable called history_type is a static string. https://github.com/mate-desktop/mate-power-manager/blob/1308a5b93d1736e605091107fcbc95dff1273635/src/gpm-statistics.c#L46

rbuj commented 2 years ago

@mbkma done

mbkma commented 2 years ago

Thanks, yeah I noticed that you were right. I have not time to test this right now, I hope to have more time soon.

raveit65 commented 2 years ago

What a huge PR, how can this be reviewed?

rbuj commented 2 years ago

Just compile the code with Address Sanitizer enabled. Next, run mate-power-statistics and mate-power-preferences from the command line.

mate-power-preferences: It fixes the memory leak in mate-power-preferences, because missing g_option_context_free.
mate-power-statistics: gpm_upower_get_device_icon function returns a new allocated string. https://github.com/mate-desktop/mate-power-manager/blob/2a42090ca95cb39dc42f0de0cc3e56b244777446/src/gpm-upower.c#L174
mate-power-statistics: Instead of storing a string using a global variable (history_type & stats_type), use an enum, as working with enums is more efficient, avoiding the memory leak.

g_settings_get_string returns a new allocated memory, not a const gchar *:

before

static const gchar *history_type;
static const gchar *stats_type;

history_type = g_settings_get_string (settings, GPM_SETTINGS_INFO_HISTORY_TYPE);

stats_type = g_settings_get_string (settings, GPM_SETTINGS_INFO_STATS_TYPE);

raveit65 commented 2 years ago

It seems that those leaks are fixed in my test.

[rave@mother ~]$ /usr/local/bin/mate-power-statistics 

=================================================================
==84469==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 26 byte(s) in 2 object(s) allocated from:
    #0 0x7fe86231591f in __interceptor_malloc (/lib64/libasan.so.6+0xae91f)
    #1 0x7fe86114e938 in g_malloc (/lib64/libglib-2.0.so.0+0x5d938)

Direct leak of 14 byte(s) in 1 object(s) allocated from:
    #0 0x7fe86231591f in __interceptor_malloc (/lib64/libasan.so.6+0xae91f)
    #1 0x7fe860e209cf in __vasprintf_internal (/lib64/libc.so.6+0x7e9cf)

[rave@mother ~]$ /usr/local/bin/mate-power-preferences 

=================================================================
==85806==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 88 byte(s) in 1 object(s) allocated from:
    #0 0x7faee4df6ad7 in calloc (/lib64/libasan.so.6+0xaead7)
    #1 0x7faee3c2fe60 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5de60)

Indirect leak of 112 byte(s) in 1 object(s) allocated from:
    #0 0x7faee4df6ad7 in calloc (/lib64/libasan.so.6+0xaead7)
    #1 0x7faee3c2fe60 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5de60)

Indirect leak of 61 byte(s) in 3 object(s) allocated from:
    #0 0x7faee4df691f in __interceptor_malloc (/lib64/libasan.so.6+0xae91f)
    #1 0x7faee3c2f938 in g_malloc (/lib64/libglib-2.0.so.0+0x5d938)

lukefromdc commented 2 years ago

Testing this on the converted Chromebook, mate-power-statistics works fine, as do options in the "on battery power" tab in the preferences dialog. Will merge