Correctness fixes (memory leaks, compiler warning, out-of-bounds read) #411
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When running bemenu like:
env BEMENU_OPTS="--list 3" bemenu
Valgrind will report an out-of-bounds read:
Invalid read of size 1
at 0x10BC91: do_getopt.part.0 (common.c:366)
by 0x10C635: do_getopt (common.c:340)
by 0x10C635: parse_args (common.c:556)
by 0x10B535: main (bemenu.c:55)
Address 0x4ac13e2 is 0 bytes after a block of size 2 alloc'd
at 0x4849BF3: calloc (vg_replace_malloc.c:1675)
by 0x10C533: cstrcopy (common.c:120)
by 0x10C533: tokenize_quoted_to_argv (common.c:146)
by 0x10C60C: parse_args (common.c:555)
by 0x10B535: main (bemenu.c:55)
The problem is that the parsing code for `--list` will blindly compare
a character past the number of lines to parse for e.g. `--list '3 up'`
but the end of the string may come right after the number of lines.
In my system Valgrind does not find the error when running bemenu like
`bemenu --list 3` even though the logic is equally questionable.
Fix it by checking that there is more after the number of lines.
GCC 14 added a new warning (included in `-Wextra`) when calling `calloc` with `sizeof` in the first argument and the number of elements in the second argument. See: https://gcc.gnu.org/onlinedocs/gcc-14.1.0/gcc/Warning-Options.html#index-Wcalloc-transposed-args Note that this is just a coding style warning.
We own the Pango-Cairo context and must release it.
The following resources were not being released: - `wl_seat` - `wl_output` - `wp_fractional_scale_manager_v1` - `wp_viewporter`
Make sure that we release the memory used by each `wl_list` node that was previously allocated with `calloc`. I'm not sure if freeing the memory of the nodes without removing it from the `wl_list` first could cause any problem so prefer removing it from the list before releasing the memory.
The existing logic leads to leaks in several scenarios: 1. Neither `wl_pointer_destroy` nor `wl_touch_destroy` are ever called, so pointer and touch objects are never released. 2. The logic for `wl_keyboard_destroy` is not solid as seats can lose the `WL_SEAT_CAPABILITY_POINTER` capability without losing the `WL_SEAT_CAPABILITY_KEYBOARD`. Thus it's possible for the calls to `wl_seat_get_keyboard` and `wl_keyboard_destroy` to be unbalanced. 3. Those resources were not released in the renderer destructor. This reworks the seat resource release logic in a systematic way to ensure that seat resources are always released.
|
Looks good to me. Thank you. |
Earnestly
reviewed
Jul 7, 2024
| @@ -363,7 +363,7 @@ do_getopt(struct client *client, int *argc, char **argv[]) | |||
| { | |||
| char *ptr; | |||
| client->lines = strtol(optarg, &ptr, 10); | |||
| client->lines_mode = (!strcmp(ptr + 1, "up") ? BM_LINES_UP : BM_LINES_DOWN); | |||
| client->lines_mode = (*ptr && !strcmp(ptr + 1, "up") ? BM_LINES_UP : BM_LINES_DOWN); | |||
There was a problem hiding this comment.
Should this not be ptr instead of *ptr if the purpose is to check if ptr is not NULL?
There was a problem hiding this comment.
The idea is not to check if ptr is not NULL (in fact, strtol guarantees that ptr is not NULL), but rather to check if strtol parsed the entire string, i.e. if optarg consists of only digits (e.g. optarg = "3").
If that's the case then ptr will point to the NUL terminator, so *ptr = '\0' = 0 and after the changes strcmp(ptr + 1, ...) will not be called.
Previously, in this case it was calling strcmp(ptr + 1, ...), which was an out-of-bounds read because ptr + 1 is right past the NUL terminator.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a small pack with fixes for 4 memory leaks found using Valgrind, a harmless GCC 14 compiler warning, and an out-of-bounds read when parsing the --list command line argument. None of the changes should affect user-visible behaviour.
The "wayland: Fix seat resource release logic" commit is a bit more interesting because it not only leaks memory, but it can also cause event handlers (e.g.
keyboard_handle_key) to be called multiple times. I think this is harmless in the current bemenu implementation, but it caused issues on a branch I am working on.