Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace realloc with g_realloc in icon-lookup.c #1182

Merged
merged 2 commits into from
Jul 1, 2023

Conversation

Costinteo
Copy link
Contributor

@Costinteo Costinteo commented Jun 12, 2023

The purpose is to avoid any failure in realloc, leading to null pointer dereferences/memory leaks. Failure in g_realloc will also terminate Dunst.
@bebehei, waiting for any further suggestions/reviews. :)

@bebehei
Copy link
Member

bebehei commented Jun 13, 2023

Hey @Costinteo, I've checked the code. The problems you found are fixed, but this PR will solve a fraction of the memory problems.

Why is the calloc in the same file not problematic? See here:

icon_themes[index].dirs = calloc(icon_themes[index].dirs_count, sizeof(struct icon_theme_dir));

@Costinteo Costinteo force-pushed the master branch 2 times, most recently from fcae097 to 1c6b1b0 Compare June 13, 2023 16:53
@Costinteo
Copy link
Contributor Author

Costinteo commented Jun 13, 2023

@bebehei, following your comment, I've analysed the rest of the code base and replaced all alloc functions with their GLib counterparts.
calloc calls have been specifically replaced with g_malloc followed by a memset to 0. Except for this case, where the memory was initialised anyway, in the next few lines:
https://github.com/Costinteo/dunst/blob/master/src/icon-lookup.c#L130

EDIT: Just noticed there's a g_malloc0 in GLib. I'll be replacing the g_malloc + memset pairs with that, instead.
EDIT2: Done. You can look through it.

@fwsmit fwsmit merged commit 91f3b9c into dunst-project:master Jul 1, 2023
@fwsmit
Copy link
Member

fwsmit commented Jul 1, 2023

calloc should be replaced with gmalloc0_n, not gmalloc0, so I fixed that. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants