Hi, this PR fixes this error spotted in Valgrind:
``` Invalid read of size 1 at 0x49211F3: utils_get_initials (utils.c:775) by 0x48EBA56: load_dialog_prefs (keyfile.c:1025) by 0x48EBA56: read_config_file (keyfile.c:1258) by 0x48ED7C7: configuration_load (keyfile.c:1286) by 0x48EFBB7: load_settings (libmain.c:917) by 0x48EFBB7: main_lib (libmain.c:1154) by 0x5FC7CCF: (below main) (libc_start_call_main.h:58) Address 0xd21bfd1 is 0 bytes after a block of size 1 alloc'd at 0x4843788: malloc (vg_replace_malloc.c:442) by 0x5B26902: g_malloc (gmem.c:100) by 0x5B1BC2A: g_key_file_parse_value_as_string (gkeyfile.c:4310) by 0x5B1BD71: g_key_file_get_string (gkeyfile.c:1965) by 0x49213FF: utils_get_setting_string (utils.c:900) by 0x48EBA29: load_dialog_prefs (keyfile.c:1023) by 0x48EBA29: read_config_file (keyfile.c:1258) by 0x48ED7C7: configuration_load (keyfile.c:1286) by 0x48EFBB7: load_settings (libmain.c:917) by 0x48EFBB7: main_lib (libmain.c:1154) by 0x5FC7CCF: (below main) (libc_start_call_main.h:58) ```
I slightly reshaped `utils_get_initials()` to make it kinda more intuitive. It also got improved a bit, in that it now produces correct results when `name` begins with a space (which AFAICT doesn't really occur, but why not...) You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3844
-- Commit Summary --
* Fix read past end of string in utils_get_initials()
-- File Changes --
M src/utils.c (12)
-- Patch Links --
https://github.com/geany/geany/pull/3844.patch https://github.com/geany/geany/pull/3844.diff
This is a function that has only a single user but has been pulled out and put in another file to make life more complicated instead of doing it locally where the conditions of the parameters are visible [end rant].
The overflow is when name is `\0` and the use probably thinks it is never `\0` by using the user real name if it is, so the past the end won't happen, but I suppose its possible (though very very unlikely) for a user real name to be empty, so yeah why not be safe.
Yes the thought is exactly that since this is (for better or for worse) a util function, it has to be robust against all types of inputs.
But do note that this overflow read does actually occur, when `Preferences -> Templates -> Developer` is empty (= `\0`).
this is (for better or for worse) a util function
Well only within Geany, its not available to plugins. Its just located in a file called `utils.c` but its not really a util, it seems unlikely that any other part of Geany will need a function that returns the _byte_ (not character) after ASCII space, I do hope Åsa Åkesson does not use Geany templates :grin:
But do note that this overflow read does actually occur, when Preferences -> Templates -> Developer is empty (= \0).
Damn users, fancy deleting their name!!!
Good find! But I believe the fix, although not bad (despite me not liking the `strlen()` use here, but that hardly matters) is not nearly enough. This function is also *terrible* with non-ASCII handling, to the point where it does more harm than good.
I propose an alternate (yet a bit over-the-top) fix in #3846.
Closed #3844 via #3846.
github-comments@lists.geany.org