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 undefined behavior on destroying an event context #709

Merged
merged 1 commit into from Jul 22, 2016

Conversation

jyrkive
Copy link
Member

@jyrkive jyrkive commented Jul 22, 2016

Quoting the corresponding messages in IRC:

[19:49:36] FYI: the game crashed when I tried to open the Preferences dialog ingame. Looks related to the event context changes.
[19:49:37] https://gist.github.com/jyrkive/3992782bfb0c7b6bea6f9dd2ac6b9dad
[19:49:42] I'm investigating.
[20:10:19] :o
[20:21:54] * stikonas (~gentoo@wesnoth/translator/stikonas) has quit IRC (Quit: Konversation terminated!)
[20:30:50] All right, the problem is in the destructor of the context class.
[20:30:51]

wesnoth/src/events.cpp

Lines 133 to 146 in 681322b

context::~context()
{
if (!handlers.empty()) {
for (handler_list::iterator it = handlers.begin(); it != handlers.end(); ++it) {
if ((*it)->has_joined()) {
(*it)->has_joined_ = false;
}
if ((*it)->has_joined_global()) {
(*it)->has_joined_global_ = false;
}
it = handlers.erase(it);
}
}
}

[20:31:43] It performs "it = handlers.erase(it);" followed by "++it" in every iteration. It other words, it increments it twice.
[20:32:24] If the number of handlers happens to be odd, the second increment increments the end iterator, which is undefined behavior.
[20:32:46] Microsoft's debug CRT is guaranteed to crash in that situation, fortunately.
[20:32:46] I think logically the ++it is not supposed to be there.
[20:33:14] It doesn't make sense to manually remove the handlers from the list anyway.
[20:33:16] Yeah, MSVC tends to be pretty good about crashing in these sorts of situations.
[20:33:29] Hmm.
[20:33:42] This is the destructor of the context class. The destructor of the list is just about to be executed, and that would clear the list for us.

The destructor of the context class accidentally incremented the iterator
twice per iteration. If the number of event handlers was odd, the
destructor ended up incrementing the end iterator, which is UB.

I rewrote the whole destructor. It's unnecessary to manually remove event
handlers from the list because the list will do it automatically when it's
destroyed.
@aginor aginor merged commit b9b97ba into wesnoth:master Jul 22, 2016
@jyrkive jyrkive deleted the fix-event-context-crash branch July 23, 2016 03:55
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

2 participants