mapeditor / tiled

Flexible level editor
https://www.mapeditor.org/
Other
11.26k stars 1.76k forks source link

Segfault when attempting to operate on a world accessed through a directory symlink #4042

Closed blujai831 closed 2 months ago

blujai831 commented 2 months ago

Describe the bug Attempting to operate in any meaningful way on a world accessed through a directory symlink will always cause a segfault. It's possible to open the world, but only the active map will be displayed, not the whole world, and if Tiled has been relaunched since the world was created, then switching to the world tool will cause an immediate segfault, as will closing the world. Since it's not possible to close the world through Tiled's interface, external measures are necessary to recover a usable session, e.g. editing the session file by hand, or deleting the offending directory symlink so that Tiled can't load the world on relaunch.

To Reproduce Steps to reproduce the behavior:

  1. Boot a Linux environment with Tiled installed.
  2. Open the terminal.
  3. If your terminal or shell is configured to start somewhere other than your home directory, run cd (or adjust remaining steps for use with your preferred working directory).
  4. Run mkdir foo. Adjust filename as needed if something already exists at foo.
  5. Run ln -s foo bar. Adjust filename as needed if something already exists at bar.
  6. Place an image file in /home/$USER/foo. (Replace $USER with your username.) Any image should do.
  7. Launch Tiled.
  8. Create a tileset in /home/$USER/bar using the image file there. Be sure to create the tileset in bar, not in foo, even though we know they're actually the same location.
  9. Create two maps in /home/$USER/bar which use the tileset.
  10. Create a new world in /home/$USER/bar.
  11. Add both maps to the world. Notice that even with the world tool active, each tab now shows only the associated map, not the world.
  12. Exit. Save changes to the world when prompted to do so.
  13. Relaunch Tiled. If you closed the tileset, maps, and/or world, reopen them. Be sure to open them through bar, not foo.
  14. Switch to the world tool. Tiled should now segfault.
  15. Relaunch Tiled. It should restore the session you closed out of.
  16. Try to unload the world or close all files open in the session. Tiled should now segfault again.
  17. Relaunch Tiled and observe that the uncooperative world is still open.
  18. Close Tiled and delete /home/$USER/bar.
  19. Relaunch Tiled and observe that the frequent crashes have abated.

Expected behavior The behavior I would expect would be identical to that presented in the case that the path does not include a directory symlink: namely, the initial action of adding both maps to the world should cause display of both maps next to each other, rather than only one map, and then, switching to the world tool after relaunching should allow the user to move the maps relative to each other, rather than crashing.

Media Not applicable; issue occurs regardless of the graphics involved.

Specifications:

bjorn commented 2 months ago

Thanks for the detailed report! I could immediately reproduce the issue by following your steps.

In a debugger I noticed the issue is due to the world storing its own file name as the "canonical path", but the lookup of a WorldDocument instance is in some cases done using a non-canonical path. Since the code assumed that this lookup will not fail for loaded worlds, it crashes on the failed lookup in DocumentManager::ensureWorldDocument.

While I would be tempted to fix this by always using the canonical path, I believe resolving symlinks has bitten me in the past. This is why for maps and tilesets, Tiled remembers the "absolute path" separately from the "canonical path" and only does the lookup of existing loaded assets by the canonical path, while doing all regular file operations using just the "absolute path". Likely a similar change needs to be done for worlds.

bjorn commented 2 months ago

I think I got the crash fixed in #4043 (well, that part of the code is gone, so...). But since it turned into such a huge change, it might also break some new things, so it will still need testing.

It also might not fix other existing bugs related to using a symbolic link. I haven't checked the behavior carefully yet, but I still did not see both maps in the world as I think should be expected.

bjorn commented 2 months ago

Alright, in the latest version of #4043, your use-case should now work. I had to change two more locations to not use the canonical file path (https://github.com/mapeditor/tiled/pull/4043/commits/4e1f3ac0b7ee0698660fe14d531eaac1d8a84a34). And as I'm pointing out in that commit, I'm starting to doubt whether I should ever use the "canonical" file path.

As it stands, retrieving the canonical path allows for example to switch to an already loaded map or tileset regardless of which symlink you use to open it. When not doing this, each different path would open as if it was a different file. And then, when changing and saving any of these, you'd probably get a reload on all the others (or a warning, when those editors also have changes).

Anyway, I hope the stuff works kinda alright now for worlds.