`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
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3536
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany/pull/3...@github.com>
