DescentDevelopers / Descent3

Descent 3 by Outrage Entertainment
GNU General Public License v3.0
2.88k stars 251 forks source link

Fixed access of map::iterator to recently deleted element #608

Closed pzychotic closed 1 month ago

pzychotic commented 1 month ago

Pull Request Type

Description

Deleting an element from a std::map invalidates the iterator to that element. So trying to continue the loop will lead to bad things. Fortunately map::erase() returns an iterator to the next element that we can assign to it to correctly continue the loop. Also removed a left over cast of basename to std::filesystem::path.

Related Issues

Fixes #604

Screenshots (if applicable)

Checklist

Additional Comments

sebholt commented 1 month ago

This fixes the invalid iterator usage.

But doesn't this skip the element after the erase because the for loop increments the iterator right after the erase?

This feels more correct to me

  for (auto it = OSIRIS_Extracted_scripts.begin(); it != OSIRIS_Extracted_scripts.end();) {
    if (mission_only && (!(it->second.flags & OESF_MISSION))) {
       ++it;
    } else {
      std::filesystem::remove(OSIRIS_Extracted_script_dir / it->second.temp_filename);
      it = OSIRIS_Extracted_scripts.erase(it);
    }
  }
pzychotic commented 1 month ago

Ups! Of course, you're right. Thanks for the catch!

winterheart commented 1 month ago

Always failing on deleting iterator... Thanks for reporting to @Jayman2000 and proposed fix. Retested on Windows for sure.