#1033 closed defect (fixed)
SKY: Suspicious memory access in RNC decoder
| Reported by: | eriktorbjorn | Owned by: | |
|---|---|---|---|
| Priority: | normal | Component: | Engine: Sky |
| Version: | 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 , 21 years ago
comment:2 by , 21 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 , 21 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:4 by , 19 years ago
| Component: | → Engine: Sky |
|---|---|
| Game: | → Beneath a Steel Sky |
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. :-)