segfault when trying to preload invalid file

Bug #1805042 reported by Arty
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
widelands
Won't Fix
Undecided
Unassigned

Bug Description

I was trying to find ways to accidentally trigger errors Widelands, and I stumbled upon a crash:

1. Copy some non-empty file (which isn't a Settler-2-map) into your maps directory and change its extension to ".swd".

2. Open the Widelands editor, then open the Load or Save dialogue (and open the directory with that file if not already in there).

Result (in most cases): The application crashes (segmentation fault). No error message, no log output.

Weirdly when I started adding some output in src/editor/ui_menus/main_menu_load_or_save_map.cc (basically just to confirm that the problem is with the preloader, not somewhere before or after), this made the crash disappear. (Instead, the preloader just threw some exception, it was caught, the file was deemed invalid and was not listed, exactly as it should be.)

I assume the preloader tried - based on some invalid data it read - to access memory it wasn't allowed to. (And when I had added some output, the addressed memory coincidentally was "safe" because part of memory used by the game.)

Not sure how serious we want to take this. I wouldn't really expect any invalid S2 maps to accidentally land in any Widelands map folder. Maybe this can also happen for Widelands maps though, then this scenario might occur more easily.

I'm not sure how much time I have next week, but I'll try to go over the preloaders and add some validity checks where necessary.

Tags: filesystem

Related branches

Revision history for this message
GunChleoc (gunchleoc) wrote :

It is always possible that files can become corrupt, so a check is a good idea :)

tags: added: filesystem
Revision history for this message
Arty (artydent) wrote :

Finally got some time for Widelands again. Checked the preloader properly and found a few potential problems that can (for invalid files) lead to out-of-bounds operations.
What caused the crashes was likely an invalid player number which then lead to out-of-bounds access of the player name array (and possibly other stuff).
It was also possible too read too much data (possibly the whole file) when the null-terminator for name or author was missing. (At least that one would not have been critical, because our fileread data artificially ends with a null.)

I added a general validity check for the header (or rather for the header part we use) to avoid those issues. (And to generally sort such invalid files out, regardless of crash potential.) Branch is up.

I looked (only briefly) over the loader for the complete S2 map, but this didn't seem to have those kinds of problems. I also looked (briefly) over loaders of our Widelands maps, and it seems that this is already handled in a safer way. Don't have time right now to look properly at that, but I don't even consider this too important.

GunChleoc (gunchleoc)
Changed in widelands:
milestone: none → build21-rc1
status: New → Confirmed
Revision history for this message
GunChleoc (gunchleoc) wrote :
Changed in widelands:
status: Confirmed → Won't Fix
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.