#8109 closed patch
DIG: Experimental memory corruption fix
| Reported by: | eriktorbjorn | Owned by: | SF/ender |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | The Dig |
Description
This patch is experimental in two senses: First of all, it's not very well tested, and secondly I'm sort of hoping someone will tell me it's wrong and that there's a much better way to do it.
The problem with the Dig memory corruption is, I think, that whenever we use the mask buffer, we assume that it has one strip for each strip in the room image (in the case of the Dig's nexus room, that's more than 220), yet we only allocate enough memory for the strips visible on screen (typically 40), and we assume that's also the mask buffer's pitch.
What this patch does is simply to always allocate enough space for 240 strips in the mask buffer, and change the pitch assumption, which means it will use six times as much memory as before. In most cases, that seems like an unnecessary waste of space to me, so I'd really like to find a better way.
Note that even if this patch is rejected, there are still some places in string.cpp and gfx.cpp where the number of strips is hard-wired as 40. That, at least, should be fixed.
Ticket imported from: #629043. Ticket imported from: patches/214.
Attachments (4)
Change History (20)
by , 22 years ago
| Attachment: | dig-corruption.diff added |
|---|
comment:1 by , 22 years ago
Some questions: * you talk about allocation, yet I don't see in the patch that the allocation of rtBuffer 9 in gfx.cpp is affected. it is still based on gdi._numStrips, so w/o having tested the patch yet, it'd seem as if we now actually started to overwrite more memory than we did before ?!? what am I missing?
* we actually always only need 40 strips on the screen (well, _numStrips, but let's keep it simple here :-). So, wouldn't it be possible to stick with 40 strips, but rather modify whatever code is incorrectly accessing the mask buffer to take _screenStartStrip into account?
I was investigating in a similar direction, but currently have no access to a system with SDL installed, so I can't test anything right now :-/
comment:2 by , 22 years ago
Thinking about my idea again, this probably wouldn't work due to charset masks, no? I.e. if text occurs on the screen the mask has to be adjusted, and if scroll back and forth while the text is on screen, the mask data would be lost (I assume this can happen, can't it?).
But I am still puzzlzed by that call to createResource(rtBuffer, 9).
Neither does your patch affect it, nor should it work correctly for any
Scumm game that has rooms bigger than a single screen (that is, it
should cause memory corruption for all of them). Maybe I am really
missing something big, but w/o any way to test my theories, I am
restricted to rambling here in the hope that Erik maybe can distill one or
another useful idea 8-)
comment:3 by , 22 years ago
> But I am still puzzlzed by that call to createResource(rtBuffer, 9). > Neither does your patch affect it,
Surely it does? It changes the itemsize from <something> * gdi._numStrips to <something> * MAX_NUM_STRIPS, and the mask buffer size is itemsize * gdi._numZBuffer. MAX_NUM_STRIPS is 240, while gdi._numStrips is likely to be 40.
> nor should it work correctly for any Scumm game that has rooms > bigger than a single screen (that is, it should cause memory > corruption for all of them).
That was what I thought too, but note that everywhere where we use the mask buffer we still assume that its *pitch* is 40 strips, so we would probably never write more than, say, 200 bytes outside the buffer, and even then only in the case where we write to the *last* mask buffer. Which would explain why Endy's hack to decrease the mask height by one worked.
So usually we have at least one whole mask buffer as a "safety zone".
I agree that in theory we'd only need 40 strips for the mask buffer, but I don't know enough to actually implement this. A simpler approach would be to make the mask buffer as wide as the room itself, but I don't even know enough to do that. :-)
comment:4 by , 22 years ago
Yeah, increasing the memory usage that much is just unacceptable really -
we're already bloaty enough :)
Altering the presumption that we need the WHOLE mask buffer all the time
is the best fix, really. Fingolfin is right, in that it means we'd need to
re-decompress the mask data every time the room position changes, which
might make scrolling a little jerky..
But either way, there has to be a better fix than this. Hmm.
comment:5 by , 22 years ago
As I said, I'm hoping for this patch to be rejected in favor of something better. It's good to know that we're all agreeing on this point. ;-)
Are we also agreeing on the original reasoning behind the patch, i.e. that faulty mask buffer handling is what's causing the memory corruption?
comment:6 by , 22 years ago
Well I can't provoke the nexus crash no matter what I am doing, so I can't say much about this. I see, though, that the current code would override memory, which is why I agree it needs to be fixed.
But Ender, seriously, look at a mem report for ScummVM, we are vastin a *lot* more memory in a couple other spaces :-)
Re-decompressing the room mask might not be enough, unless you plan to also check if any text is displaying and then putting up a charset mask for that, too.
comment:7 by , 22 years ago
I haven't checked how much memory ScummVM uses, really. :-)
In case of the Nexus, the mask buffer is currently about 24 Kb. With the patch, it's six times as large. As I've indicated earlier, what I would have liked to do is to make the mask buffer only as wide as the room itself. That way the memory consumption should stay almost the same as before most of the time.
The reason I didn't was that I couldn't figure out how to get the width of the current room. Any hints?
comment:8 by , 22 years ago
Here's an updated patch that bases the width of the mask buffer on _scrWidth instead of some theoretical max width. As fara as I can tell, it works.
One thing though: Could anyone tell me what the heck restoreBG() is trying to do to the mask buffer, because that code makes little sense to me.
comment:9 by , 22 years ago
To elaborate a bit, restorebg.diff shows what I would have expected restoreBG() to do with the mask buffer. It seems to work for the cases where I've tested it, but I'm not aware of any case where vs->topline != 0 so that part is still a bit of a mystery to me.
comment:10 by , 22 years ago
And since no one has yet made any convincing case for my insanity, dig-corruption3.diff combines dig-corruption2.diff and restorebg.diff.
comment:11 by , 22 years ago
replacing _realWidth with _scrWidth is only hack, which only increasing buffer, to prevent memory corruption
comment:12 by , 22 years ago
Aquadran, I see you checked in a patch for the Dig nexus problems. Does that mean this patch is obsolete, with the possible exception of restorebg.diff?
I'm hoping you'll say yes, even if that would mean that I still don't quite understand how the mask buffer is handled. ;-)
comment:13 by , 22 years ago
yes :-) patch for nexus problem is obsolete now, exception restorebg.diff
comment:14 by , 22 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:16 by , 6 years ago
| Component: | → Engine: SCUMM |
|---|---|
| Game: | → The Dig |
Patch against an October 25 CVS snapshot