Opened 16 years ago

Closed 16 years ago

Last modified 14 years ago

#1033 closed defect (fixed)

SKY: Suspicious memory access in RNC decoder

Reported by: eriktorbjorn Owned by:
Priority: normal Component: Engine: Sky
Keywords: Cc:
Game: Beneath a Steel Sky

Description

I've tried running BASS (v0.0372) with both Valgrind
and Electric Fence, and they both seem to agree that
there are times when the RNC decoder reads outside of
its allocated buffer. Here's a stack trace of one such
case:

#0 RncDecoder::inputBits(unsigned char)
(this=0xbfffe520, amount=7 '\a') at scummsys.h:378
#1 0x080cbf1a in RncDecoder::inputValue(unsigned
short*) (this=0xbfffe520, table=0x40b52ffd) at
sky/rnc_deco.cpp:154
#2 0x080cc1d0 in RncDecoder::unpackM1(void const*,
void*, unsigned short) (this=0xbfffe520, input=0xa,
output=0x40b54efa, key=0) at sky/rnc_deco.cpp:244
#3 0x080c6b49 in SkyDisk::loadFile(unsigned short,
unsigned char*) (this=0x40b0ce5c, fileNr=11910,
dest=0x40b54efa "") at sky/disk.cpp:199
#4 0x080cb5d1 in SkyMouse (this=0x412fcfe0,
system=0x40b52fff, skyDisk=0x40b52fff) at sky/mouse.cpp:87
#5 0x080b7bfa in SkyState::initialise()
(this=0x40aeaf98) at sky/sky.cpp:253
#6 0x080b76b5 in SkyState::go() (this=0x40aeaf98) at
sky/sky.cpp:176
#7 0x080da661 in main (argc=2, argv=0xbffffb94) at
common/main.cpp:230

Could this have any bearing on the random crashes that
some people - me included - have been seeing?

The good news is that this particular one happens when
loading MICE_FILE, which appears to be pretty small.
That should make it easier to understand what's going
on, right?

Ticket imported from: #771549. Ticket imported from: bugs/1033.

Change History (4)

comment:1 by eriktorbjorn, 16 years ago

While on the subject of the RNC decoder, what's the purpose
of this piece of code:

if (! ((inputHigh <= outputLow) || (outputHigh <=
inputHigh)) ) {
_srcPtr = inputHigh;
_dstPtr = outputHigh;
memcpy((_dstPtr-packLen), (_srcPtr-packLen),
packLen);
_srcPtr = (_dstPtr-packLen);
}

It looks to me as if it's determining whether or not the
input and output buffers are overlapping, but does that
really make sense? I thought they were allocated separately.

(It probably doesn't have much bearing on this bug report
though. I was just trying to get a bit more understanding of
the RNC decoder, and this was as far as I got before running
out of time. :-)

comment:2 by joostp, 16 years ago

The RNC decoder allows you to use the same buffer for
output as you use for input, so it overwrites the input when
unpacking ...hence the check.

The OOB access bug has been worked around (by allocating 4
bytes extra) for the time being, pending a complete RNC
decoder rewrite.

comment:3 by joostp, 16 years ago

Resolution: fixed
Status: newclosed

comment:4 by fingolfin, 14 years ago

Component: Engine: Sky
Game: Beneath a Steel Sky
Note: See TracTickets for help on using tickets.