Opened 9 years ago

Closed 8 years ago

Last modified 11 months ago

#5498 closed defect (fixed)

ALL: Corrupted ZIP files are not properly handled

Reported by: raziel- Owned by: Templier
Priority: low Component: GUI
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)

scummvm.ini (20.7 KB ) - added by raziel- 9 years ago.
scummmodern.zip (177.6 KB ) - added by raziel- 9 years ago.
Crippled modern

Download all attachments as: .zip

Change History (22)

comment:1 by wjp, 9 years ago

When does that happen? How do you reproduce it and do you have a backtrace?

comment:2 by raziel-, 9 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 lordhoto, 9 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:4 by digitall, 9 years ago

raziel_ ,
What platform and OS are you building on? Amiga?

comment:5 by raziel-, 9 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 fingolfin, 9 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 raziel-, 9 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

comment:8 by wjp, 9 years ago

Yes, please attach your scummvm.ini.

by raziel-, 9 years ago

Attachment: scummvm.ini added

comment:9 by raziel-, 9 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

by raziel-, 9 years ago

Attachment: scummmodern.zip added

Crippled modern

comment:10 by raziel-, 9 years ago

I have attached the crippled modern file which caused it in the first place

comment:11 by fingolfin, 9 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 fingolfin, 9 years ago

Priority: normallow

comment:13 by raziel-, 9 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:14 by bluegr, 9 years ago

Updated description

comment:15 by bluegr, 9 years ago

Summary: ALL: Assertion in common/stream.cppALL: Corrupted ZIP files are not properly handled

comment:16 by Templier, 8 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 Templier, 8 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:18 by Templier, 8 years ago

Should be fixed by 25d97593c50ae88ce1b6

comment:19 by Templier, 8 years ago

Owner: set to Templier
Resolution: fixed
Status: newclosed

comment:20 by digitall, 11 months ago

Component: GUI
Note: See TracTickets for help on using tickets.