Branch: refs/heads/master Author: Colomban Wendling ban@herbesfolles.org Committer: GitHub noreply@github.com Date: Thu, 05 Oct 2023 20:13:25 UTC Commit: 3374ea8154b007cb155afd513d6cfdb34a09e5e6 https://github.com/geany/geany/commit/3374ea8154b007cb155afd513d6cfdb34a09e5...

Log Message: ----------- Fix out-of-bounds read in foreach_ptr_array() (#3536)

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.

Modified Paths: -------------- src/utils.h

Modified: src/utils.h 3 lines changed, 1 insertions(+), 2 deletions(-) =================================================================== @@ -106,8 +106,7 @@ G_BEGIN_DECLS * @param idx @c guint index into @a ptr_array. * @param ptr_array @c GPtrArray to traverse. */ #define foreach_ptr_array(item, idx, ptr_array) \ - for (idx = 0, item = ((ptr_array)->len > 0 ? g_ptr_array_index((ptr_array), 0) : NULL); \ - idx < (ptr_array)->len; ++idx, item = g_ptr_array_index((ptr_array), idx)) + for (idx = 0; idx < (ptr_array)->len && (item = g_ptr_array_index((ptr_array), idx), TRUE); ++idx)

/** Iterates all the nodes in @a list. * @param node should be a (@c GList*).

-------------- This E-Mail was brought to you by github_commit_mail.py (Source: https://github.com/geany/infrastructure).