Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when plugin_set_key_group() is called several times by plugins #1426

Merged
merged 1 commit into from Apr 15, 2017

Conversation

techee
Copy link
Member

@techee techee commented Mar 12, 2017

When plugin calls plugin_set_key_group() several times for the same
group (when creating keybindings dynamically and needs to reset them),
it crashes with the current code the second time it gets called.

The reason is that group->plugin_keys is an array into which entries of
group->key_items point and when calling

g_ptr_array_set_size(group->key_items, 0);

it calls free_key_binding() for every item - when these items are
deallocated by g_free(group->plugin_keys) previously, calls of
free_key_binding() reference an invalid memory.

Just first resizing group->key_items (and calling free_key_binding() for
its items) and freeing group->plugin_keys afterwards fixes the problem.

When plugin calls plugin_set_key_group() several times for the same
group (when creating keybindings dynamically and needs to reset them),
it crashes with the current code the second time it gets called.

The reason is that group->plugin_keys is an array into which entries of
group->key_items point and when calling

g_ptr_array_set_size(group->key_items, 0);

it calls free_key_binding() for every item - when these items are
deallocated by g_free(group->plugin_keys) previously, calls of
free_key_binding() reference an invalid memory.

Just first resizing group->key_items (and calling free_key_binding() for
its items) and freeing group->plugin_keys afterwards fixes the problem.
@kugel-
Copy link
Member

kugel- commented Mar 29, 2017

LGTM (@b4n @elextr)

@elextr
Copy link
Member

elextr commented Mar 29, 2017

@kugel- have you tested this, I wouldn't even know where to start to trigger it?

@kugel-
Copy link
Member

kugel- commented Mar 29, 2017

No, i haven't tested it. The change is trivial and @techee explaination makes complete sense. The first post also mentions how to trigger

@elextr
Copy link
Member

elextr commented Mar 29, 2017

@techee @kugel- the "how to trigger" is in terms of code inside a plugin, which plugin?

@kugel-
Copy link
Member

kugel- commented Mar 29, 2017

Probably one that's currently in development (@techee mentions one on #1430)

@elextr
Copy link
Member

elextr commented Mar 29, 2017

Ok, so you can test it to see if it doesn't crash for other plugins.

@techee
Copy link
Member Author

techee commented Apr 3, 2017

@elextr No other plugin calls this function several times right now - at the moment all plugins just define their keybindings in the init function and they are done. So no existing plugin crashes.

@elextr
Copy link
Member

elextr commented Apr 3, 2017

ok, there are two ways of looking at this:

  1. no visible code uses it, and no other reports of the problem, and since @techee already has the fix there is no rush to commit this and it can be committed when @techee publishes his super-plugin

  2. since this function is effectively part of the code of plugin_set_key_group() (which is the only user, so the fix cannot affect other code, by inspection), and that is in the API, the bug report should be fixed before anyone else uses it

I incline to the second view, so it nobody objects within a few days will commit it.

@elextr elextr merged commit 2707006 into geany:master Apr 15, 2017
@elextr
Copy link
Member

elextr commented Apr 15, 2017

Sorry, the "few" days stretched, committed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants