Opened 17 years ago

Closed 17 years ago

Last modified 11 months ago

#8109 closed patch

DIG: Experimental memory corruption fix

Reported by: eriktorbjorn Owned by: SF/ender
Priority: normal Component: Engine: SCUMM
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)

dig-corruption.diff (7.7 KB ) - added by eriktorbjorn 17 years ago.
Patch against an October 25 CVS snapshot
dig-corruption2.diff (9.4 KB ) - added by eriktorbjorn 17 years ago.
Patch against an October 28 CVS snapshot
restorebg.diff (1.5 KB ) - added by eriktorbjorn 17 years ago.
Patch against an October 29 CVS snapshot
dig-corruption3.diff (9.9 KB ) - added by eriktorbjorn 17 years ago.
Patch against an October 31 CVS snapshot

Download all attachments as: .zip

Change History (20)

by eriktorbjorn, 17 years ago

Attachment: dig-corruption.diff added

Patch against an October 25 CVS snapshot

comment:1 by fingolfin, 17 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 fingolfin, 17 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 eriktorbjorn, 17 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 SF/ender, 17 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 eriktorbjorn, 17 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 fingolfin, 17 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 eriktorbjorn, 17 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?

by eriktorbjorn, 17 years ago

Attachment: dig-corruption2.diff added

Patch against an October 28 CVS snapshot

comment:8 by eriktorbjorn, 17 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.

by eriktorbjorn, 17 years ago

Attachment: restorebg.diff added

Patch against an October 29 CVS snapshot

comment:9 by eriktorbjorn, 17 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.

by eriktorbjorn, 17 years ago

Attachment: dig-corruption3.diff added

Patch against an October 31 CVS snapshot

comment:10 by eriktorbjorn, 17 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 aquadran, 17 years ago

replacing _realWidth with _scrWidth is only hack, which only
increasing buffer, to prevent memory corruption

comment:12 by eriktorbjorn, 17 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 aquadran, 17 years ago

yes :-) patch for nexus problem is obsolete now, exception
restorebg.diff

comment:14 by SF/ender, 17 years ago

Owner: set to SF/ender
Status: newclosed

comment:15 by SF/ender, 17 years ago

Appli0red then :)

comment:16 by digitall, 11 months ago

Component: Engine: SCUMM
Game: The Dig
Note: See TracTickets for help on using tickets.