`foreach_ptr_array()` was reading one element past the end of the array. This was not usually noticeable because the resulting garbage pointer was not actually used, and it's highly unlikely there is protected or foreign memory right after the array, but there is actually no such guarantee, and it's bad nonetheless.
This actually resulted in Valgrind complaining, and hence me noticing:
==1217514== Invalid read of size 8 ==1217514== at 0x49120B9: keyfile_action (stash.c:271) ==1217514== by 0x49130BB: stash_group_load_from_key_file (stash.c:308) ==1217514== by 0x48F179D: settings_action (keyfile.c:396) ==1217514== by 0x48F2D5E: read_config_file (keyfile.c:1245) ==1217514== by 0x48F3FAB: configuration_load (keyfile.c:1278) ==1217514== by 0x48F5393: load_settings (libmain.c:917) ==1217514== by 0x48F667F: main_lib (libmain.c:1154) ==1217514== by 0x109141: main (main.c:27) ==1217514== Address 0x910a3f0 is 0 bytes after a block of size 16 alloc'd ==1217514== at 0x48406C4: malloc (vg_replace_malloc.c:380) ==1217514== by 0x5B2C717: g_realloc (gmem.c:201) ==1217514== by 0x5AF2AA3: g_ptr_array_maybe_expand (garray.c:1640) ==1217514== by 0x5AF4066: g_ptr_array_add (garray.c:1962) ==1217514== by 0x4912247: add_pref (stash.c:491) ==1217514== by 0x491331E: stash_group_add_integer (stash.c:531) ==1217514== by 0x48F3857: init_pref_groups (keyfile.c:339) ==1217514== by 0x48F42A1: configuration_init (keyfile.c:1500) ==1217514== by 0x48F6661: main_lib (libmain.c:1146) ==1217514== by 0x109141: main (main.c:27) ==1217514==
or:
==1217514== Invalid read of size 8 ==1217514== at 0x48EA315: keybindings_foreach (keybindings.c:768) ==1217514== by 0x48EC9B4: load_user_kb (keybindings.c:817) ==1217514== by 0x48EFBDC: keybindings_load_keyfile (keybindings.c:846) ==1217514== by 0x48F6756: main_lib (libmain.c:1206) ==1217514== by 0x109141: main (main.c:27) ==1217514== Address 0xd570830 is 0 bytes after a block of size 32 alloc'd ==1217514== at 0x484582F: realloc (vg_replace_malloc.c:1437) ==1217514== by 0x5B2C717: g_realloc (gmem.c:201) ==1217514== by 0x5AF2AA3: g_ptr_array_maybe_expand (garray.c:1640) ==1217514== by 0x5AF4066: g_ptr_array_add (garray.c:1962) ==1217514== by 0x48ECC43: keybindings_set_item (keybindings.c:180) ==1217514== by 0x48ECD92: add_kb (keybindings.c:295) ==1217514== by 0x48EE686: init_default_kb (keybindings.c:518) ==1217514== by 0x48EFB7C: keybindings_init (keybindings.c:751) ==1217514== by 0x48F6698: main_lib (libmain.c:1160) ==1217514== by 0x109141: main (main.c:27)
The problematic code was setting the new value for the item pointer after incrementing the index, but before validating it was still in the valid range.
Fix this by moving the item assignment in the condition expression. This requires using a comma operator and a logical AND to make sure the expression does not contribute to the test (allowing e.g. NULL values) yet being dependent on the index validation passing.
Note that this change, as implemented here, slightly affects behavior: `item` will point to the last *actual* node of the array (not out of bounds) after the loop, but also it will not be set at all if the array has no items. Before this change, the value was NULL for no items, and garbage otherwise. As the value after the loop was effectively only usable for empty arrays, it sounds safe enough to assume no caller depended on an empty array leading to initializing `item`, so we can drop this special case. And unsurprisingly no caller in Geany itself depend on that. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3536
-- Commit Summary --
* Fix out-of-bounds read in foreach_ptr_array()
-- File Changes --
M src/utils.h (3)
-- Patch Links --
https://github.com/geany/geany/pull/3536.patch https://github.com/geany/geany/pull/3536.diff