Fix out-of-bounds read in foreach_ptr_array() (#3536)
commit3374ea8154b007cb155afd513d6cfdb34a09e5e6
authorColomban Wendling <ban@herbesfolles.org>
Thu, 5 Oct 2023 20:13:25 +0000 (5 22:13 +0200)
committerGitHub <noreply@github.com>
Thu, 5 Oct 2023 20:13:25 +0000 (5 22:13 +0200)
treeac4405c51ed65b2f7af9e1582162cdc12197400c
parent7d5ab8d14c69f253429cdb5d4ca040d525c9120e
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.
src/utils.h