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 a class of crashes in the city list #1633

Merged
merged 1 commit into from
Dec 26, 2022

Conversation

lmoureaux
Copy link
Contributor

Modifying a city can (somehow) change the selection, upon which selected_cities is modified. Modifying a QList invalidates iterators, thus we were triggering undefined behavior when doing this in loops. Copy the list before doing anything with it, which avoids any invalidation (since we work on the copy).

Also remove a duplicated nullptr check (items in the list shouldn't be nullptr anyway).

Test plan:

  • Select all cities
  • Switch to a governor that works for some cities but not all
  • Observe crash without patch (YMMV), no crash with patch

Modifying a city can (somehow) change the selection, upon which selected_cities
is modified. Modifying a QList invalidates iterators, thus we were triggering
undefined behavior when doing this in loops. Copy the list before doing
anything with it, which avoids any invalidation (since we work on the copy).
@lmoureaux lmoureaux requested a review from jwrober December 26, 2022 15:23
Copy link
Collaborator

@jwrober jwrober left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. I was able to recreate the crash and see it go away with the patch.

@jwrober jwrober merged commit b45fdf4 into longturn:master Dec 26, 2022
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.

2 participants