#5498 closed defect (fixed)
ALL: Corrupted ZIP files are not properly handled
| Reported by: | raziel- | Owned by: | Templier |
|---|---|---|---|
| Priority: | low | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
ScummVM 1.3.0svn54067 (Nov 4 2010 18:16:52) Features compiled in: Vorbis FLAC MP3 RGB zLib Theora
assertion "_pos <= _size" failed: file "common/stream.cpp", line 85
gcc (GCC) 4.2.4 (adtools build 20090118)
Ticket imported from: #3103051. Ticket imported from: bugs/5498.
Attachments (2)
Change History (22)
comment:1 by , 14 years ago
comment:2 by , 14 years ago
Happens right at the start after double-clicking ScummVM's icon.
No launcher comes up, only the SDL window/screen is opened black and the console spits that message
No backtrace as there is no "crash" per se right now, though it takes my system down rock solid pretty quickly afterwards and even now doesn't print any crash messages
sorry
comment:3 by , 14 years ago
You can actually just start ScummVM in gdb and wait for the assert to happen and then type "bt" to get a backtrace, which will show the callstack to the place where the assert took place.
comment:5 by , 14 years ago
@tdhs
D'Oh, normally that is part of my bug report, must have been late yesterday :-/
AmigaOS4, PPC, BE
@lordhoto
Unfortunately both debuggers i have access to (gdb, db101) crash itself when loading in scummvm I've noticed the authors about it, but am stuck right now
Trying a clean build now, until no further notice this stands :-/
comment:6 by , 14 years ago
Well, with so little information, there is absolutely nothing that we can do. This report right now is a bit like telling a book author "There is a typo in your book, but I can't tell you where." :).
You could at least try running ScummVM with -d9 and tell us the messages just before the assertions. Even better, start inserting printf's all over the startup code (say, inside main.cpp), to see to which place it gets, and where it crashes.
One of the first places we use streams is for loading the config file, maybe that's related. But again, it near impossible to tell.
comment:7 by , 14 years ago
Yes, i know, sorry about that :-(
The scummvm.ini was a good call though. Upon deleting ScummVM starts again, so it must be something to do with the latest change to the GUI options and the introduction of a new switch which maybe also is put down into the .ini?
If it helps i'll upload my scummvm.ini
by , 14 years ago
| Attachment: | scummvm.ini added |
|---|
comment:9 by , 14 years ago
Found the reason, it was a crippled scummvmmodern.zip file
This happens sometimes here, not sure why, it gets destroyed while being "cp"ed to it's final destination while still in sh and i know i filed at least two other bug reports earlier due to the same reason.
Awefully sorry to have kept devs busy with this
if it would be possible to warn with a dialog about messed up theme files i'd be a happy camper :-)
If not i try to remember where to look first before filing a report
sorry again
comment:10 by , 14 years ago
I have attached the crippled modern file which caused it in the first place
comment:11 by , 14 years ago
So it's not directly a bug in our code *phew*.
However, it would certainly be nice if we dealt better with the error... If we find a crippled .zip file, we should just refuse loading it (treating it as if it was no there); and maybe in addition display an appropriate error dialog. Crashing is definitely not good.
I assume in this case, we are seeking to a bogus address. Can you replace the assert by the following and tell us the output? if (_pos > _size) { error("MemoryReadStream::seek: pos=%d > size=%d", _pos, _size); }
But even with that info, not knowing the backtrace makes it very difficult to work on this.
comment:12 by , 14 years ago
| Priority: | normal → low |
|---|
comment:13 by , 14 years ago
Will do, unfortunately the crash doesn't happen anymore, not even with the build in question It simply uses the inbuilt theme now and tells me it can't find a modern theme
as soon as i get this behaviour again i will try to give more info
sorry
comment:15 by , 14 years ago
| Summary: | ALL: Assertion in common/stream.cpp → ALL: Corrupted ZIP files are not properly handled |
|---|
comment:16 by , 14 years ago
I managed to get a crash with the attached scummmodern.zip. It's different from the assertion reported in the bug though.
Here is the stacktrace: scummvm.exe!_chvalidator(int c, int mask) Line 56 + 0x2a bytes C++ scummvm.exe!isspace(int c) Line 188 + 0xb bytes C++ scummvm.exe!Common::String::trim() Line 413 + 0x1e bytes C++ scummvm.exe!GUI::ThemeEngine::themeConfigParseHeader(Common::String header, Common::String & themeName) Line 1507 C++ scummvm.exe!GUI::ThemeEngine::themeConfigUsable(const Common::ArchiveMember & member, Common::String & themeName) Line 1543 + 0x1a bytes C++ scummvm.exe!GUI::ThemeEngine::listUsableThemes(Common::Archive & archive, Common::List<GUI::ThemeEngine::ThemeDescriptor> & list) Line 1633 + 0x19 bytes C++ scummvm.exe!GUI::ThemeEngine::listUsableThemes(Common::List<GUI::ThemeEngine::ThemeDescriptor> & list) Line 1606 + 0x3f bytes C++ scummvm.exe!GUI::ThemeEngine::getThemeFile(const Common::String & id) Line 1731 + 0x9 bytes C++ scummvm.exe!GUI::ThemeEngine::ThemeEngine(Common::String id, GUI::ThemeEngine::GraphicsMode mode) Line 292 + 0x10 bytes C++ scummvm.exe!GUI::GuiManager::loadNewTheme(Common::String id, GUI::ThemeEngine::GraphicsMode gfx, bool forced) Line 135 + 0x39 bytes C++ scummvm.exe!GUI::GuiManager::GuiManager() Line 79 + 0x1c bytes C++ scummvm.exe!Common::Singleton<GUI::GuiManager>::makeInstance() Line 55 + 0x27 bytes C++ scummvm.exe!Common::Singleton<GUI::GuiManager>::instance() Line 74 + 0x5 bytes C++ scummvm.exe!setupGraphics(OSystem & system) Line 247 C++ scummvm.exe!scummvm_main(int argc, const char * const * argv) Line 382 + 0x9 bytes C++ scummvm.exe!SDL_main(int argc, char * * argv) Line 63 + 0xd bytes C++ scummvm.exe!WinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, HINSTANCE__ * __formal, HINSTANCE__ * __formal) Line 47 + 0x12 bytes C++ scummvm.exe!__tmainCRTStartup() Line 275 + 0x2c bytes C scummvm.exe!WinMainCRTStartup() Line 189 C
The call to isspace() will fail, as the data read from the THEMERC file is corrupted. Adding a basic validity check before the call to header.trim() in ThemeEngine::themeConfigParseHeader() will workaround the problem. See https://github.com/Littleboy/scummvm/commit/ea9774f689a36e967c481c56999075671d2463b7 for the patch.
The real problem is in ZipArchive::createReadStreamForMember() in unzip.cpp:1491. Notice all the calls to unz* functions without checking for any return value? In the case of the corrupted theme, the call to unzReadCurrentFile() will return UNZ_PARAMERROR, thus not initializing the buffer, thus causing the assert later on garbage data.
So a proper fix will be to check for all return values in that function and return NULL (or an empty stream). I'm not sure the rest of the code is ready to cope with that though (and it might not be a good idea to try just before branching for 1.3.0 :D)
comment:17 by , 14 years ago
Added a proper fix (see https://github.com/scummvm/scummvm/pull/25 ), but it will need a lot more testing than the simple workaround.
comment:19 by , 14 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:20 by , 6 years ago
| Component: | → GUI |
|---|
When does that happen? How do you reproduce it and do you have a backtrace?