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 iterator invalidation in panel_manager::deserialize() #29331

Merged
merged 1 commit into from
Apr 6, 2019
Merged

Fix iterator invalidation in panel_manager::deserialize() #29331

merged 1 commit into from
Apr 6, 2019

Conversation

neitsa
Copy link
Contributor

@neitsa neitsa commented Apr 6, 2019

Summary

SUMMARY: Bugfixes "Fix iterator invalidation in panel_manager::deserialize()."

Purpose of change

Discovered while debugging a Debug build which crashed in panel_manager::deserialize(); pannels.cpp

The crash didn't show up in the release build but looking at the debugger on the Debug build it was clearly coming from the debug allocator on it->toggle where the pointer validation was wrong (honestly it took me some time to understand what was the problem...).

                if( it2->get_name() == name ) {
                    if( it->get_name() != name ) {
                        window_panel panel = *it2;
                        layout.erase( it2 );
                        layout.insert( it, panel );
                    }
                    it->toggle = joPanel.get_bool( "toggle" ); // crash here!

It happens that erase() and insert() invalidate iterators and all elements after the iterator, as per the C++ reference.

Describe the solution

Simply set the current iterator as the one returned by layout.insert():

                    if( it->get_name() != name ) {
                        window_panel panel = *it2;
                        layout.erase( it2 );
                        it = layout.insert( it, panel ); // get the iterator here
                    }
                    it->toggle = joPanel.get_bool( "toggle" ); // no crash

Describe alternatives you've considered

N/A.

Additional context

After this fix and a rebuild, the code doesn't trigger a crash and the panels seems to be working as intended.

@KorGgenT KorGgenT added <Bugfix> This is a fix for a bug (or closes open issue) Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Apr 6, 2019
@ZhilkinSerg ZhilkinSerg merged commit 846cc29 into CleverRaven:master Apr 6, 2019
@ifreund
Copy link
Contributor

ifreund commented Apr 6, 2019

Thank you, that one was my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants