#3207 closed defect (fixed)
SCUMM engine abuses stack
| Reported by: | SF/khalek | Owned by: | sev- |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | detection | |
| Cc: | Game: |
Description
When trying to start any SCUMM based game the code abuses the stack, which will result in anything using a stack protector to kill the application.
For example trying to start a game on OpenBSD/amd64 4.1-current gives:
#0 0x000000004b245eea in kill () from /usr/lib/libc.so.40.3 #1 0x000000004b268ac0 in __stack_smash_handler ( func=0x9af940 "void Scumm::detectGames(const FSList&, Common::List<Scumm::DetectorResult>&, const char*)", damaged=6) at /usr/src/lib/libc/sys/stack_protector.c:89 #2 0x000000000041691b in detectGames (fslist=@0x7f7ffffc4650, results=@0x7f7ffffc4680, gameid=0x46b67f50 "monkey") at engines/scumm/detection.cpp:443 #3 0x0000000000417b9c in Engine_SCUMM_create(OSystem*, Engine**) ( syst=0x4a609800, engine=0x7f7ffffc4868) at engines/scumm/detection.cpp:791 #4 0x000000000041537a in StaticPlugin::createInstance(OSystem*, Engine**) const (this=0x50354280, syst=0x4a609800, engine=0x7f7ffffc4868) at base/plugins.cpp:49 #5 0x000000000040c7eb in runGame (plugin=0x50354280, system=@0x4a609800, edebuglevels=@0x7f7ffffc4bd0) at base/main.cpp:140 #6 0x000000000040d24a in scummvm_main (argc=4, argv=0x7f7ffffc4cb8) at base/main.cpp:297 #7 0x000000000040a5d1 in main (argc=4, argv=0x7f7ffffc4cb8) at backends/platform/sdl/sdl.cpp:121
This does not happen with other engines, this did not happen with 0.9.1.
I do not have time to do the figuring out where it broke dance right now, but clearly it is broken.
Ticket imported from: #1726330. Ticket imported from: bugs/3207.
Attachments (1)
Change History (13)
comment:1 by , 18 years ago
| Owner: | set to |
|---|
comment:2 by , 18 years ago
comment:3 by , 18 years ago
It is char md5str[32+1] and the subsequent call to md5_file_string() The string handling is totally gross, again I really don't have time due to end of semester tasks to deal with this (ie rewrite it cleanly).
comment:4 by , 18 years ago
The string handling is gross? Hm, even upon re-reviewing the code (this was one of the spots that looked most suspicious to me earlier today), I fail to see any actual problems. I just made a small paranoia commit, but I'd be surprised if it helped any.
I'd appreciate if you could enlighten me and explain what's so bad about the code :-).
comment:5 by , 18 years ago
Unfortunately I have no slightest idea why it happens. Moreover, it works very well on my box, as well as under Windows and bunch of other devices. I.e. that code lives in ScummVM since 0.9.0 when Max made a big overhaul of it.
If stack gets corrupted then application should crash as there will be no correct function return, or is is something else?
Another suspicion is a compiler bug.
Also I will stay from assigning this bug release critical status as nature of it is yet unknown. I.e. further testing is required.
I will investigate, perhaps there are some stack checking libraries fro FreeBSD exist.
comment:6 by , 18 years ago
| Priority: | blocker → normal |
|---|
comment:7 by , 18 years ago
Having a release that doesn't run SCUMM games is rather poor taste don't you think?
Here is a diff that is both more efficient and makes such games actually run here.
The string code does no size checks and does unrequired loops among other things. File Added: d.diff
comment:8 by , 18 years ago
Thank you for investigating it. The patch has been committed.
And yes, that code is problematic. md5str is being accessed when it is removed from heap.
comment:9 by , 18 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:10 by , 18 years ago
Odd, I don't understand why that change (moving the definition of md5str up) helps... Maybe I am misunderstanding something about scoping here? Please don't get me wrong, but I just would like to understand what was wrong here so that I can avoid the problem in the future.
As I recall, the code used to be: { char md5str[32+1]; if (FOO) { *copy* md5str to a Common::String object ... } }
So the way I learned scoping, the string should still be alive inside the inner if() -- and hence there should be no access to it after it was "removed" from the stack.
As for the code in md5_file_string being inefficient: Aye, it can be done w/o loops and w/o s(n)printf, but the rationale behind that code was that it's (a) very easy to understand and (b) the speed impact is completely negligible compared to the time it takes to compute the actual MD5, and (c) a few dozen millisecs (microsecs?) won't really be noticed, even when doing a full file system scan (we only compute very few MD5s).
Lack of size checks: Well, of course the code will not work properly if a buffer smaller than 33 bytes is passed to it. But there is no way to avoid that when using a char buffer. Other than that, I don't see any missing size checks?
comment:11 by , 18 years ago
Looks odd indeed. And that code was very similar in 0.9.1 (detectGames in plugin.cpp) with the same scoping or so for the md5str variable.
It would be interesting to know which compiler was used. If it's gcc2.95.x, Common::String::_storage used to cause problems with it in the past.
comment:12 by , 6 years ago
| Component: | → Engine: SCUMM |
|---|
So we need some way to track down at which point precisely the stack gets smashed. I just had a brief look through detectGames, but didn't notice anything that struck me as potential cause. Well, not surprising, since I wrote most of that code, and hence probably are rather blind for most logic bugs in it :-/.
Does Valgrind offer any help when it comes to debugging stack errors?