C++: array indexing issues

[KingMike]

I can't have a long int as the index into an array of chars?
Just curious about a graphics decompressor I had previously written.

Code Select Expand


unsigned char readbyte, readbyte2, TypeKey, LZIterations, IterationKey;
unsigned long int romoff, WriteIndex;
unsigned short int DecompIterations, LZKey, LZReadIndex, Write=0x24;
unsigned char DataBuffer[0x10024];

(the odd size is because the original game code deliberately underflowed the decompression scratchpad position with 0x24 zeroed bytes to set the read position behind the write position.)
Example usage

Code Select Expand


while(LZIterations>0)
{
DataBuffer[Write]=DataBuffer[LZReadIndex];
LZReadIndex++; Write++;
DataBuffer[Write]=DataBuffer[LZReadIndex];
LZReadIndex++; Write++;
LZIterations--;
}

I was using the long WriteIndex as the array index and it would crash/return corrupt data, replacing it with the short int Write made it work, but I don't know if that is correct.
I suppose I don't really NEED a 64KB decompression window (for SNES, probably even 32KB is overkill)

[Disch]

I can't have a long int as the index into an array of chars?

Of course you can. 

This sounds like stack corruption.  Switch back to using WriteIndex and change that line to this:

Code Select Expand


unsigned long int romoff, testtest[100], WriteIndex, testtesttest[100];


Does it work?  If yes, you have stack corruption.  Most likely you are stepping out of bounds of an array somewhere -- and that array is resting on the stack.

It *might* be DataBuffer ... like maybe you're using a negative index or something by mistake.


This, of course, is not a fix.  It's just diagnostic.  If you want further diagnostics, you can try replacing your arrays with some kind of container class so you have bounds checking. Or add your own bounds checking since you don't appear to be checking at all, which -- unless you know the exact size or a strict upper-bound of the size of the data you're working with, is an extremely bad idea.



EDIT:

Really... since this is C++ and not C, you should be using a container class like vector anyway for DataBuffer.  And instead of having a WriteIndex, you should just push_back the values into your vector.  Then you never have to worry about out of bounds writes -- and your buffer is not unnecessarily large.

And, if you compile with debug settings, you also get bound checking on read-accesses as well.

[Tirlititi]

Maybe a dumb question but programing errors are often too dumb for our own good: do you initialize WriteIndex before using it, as you initialize Write?

[KingMike]

Well, it seemed to have been fixed. I was sure I had initialized WriteIndex before.

I remember now the problem was that before between two writes to DataBuffer, TypeKey (a bitfield which holds the state of whether 8 iterations are raw or LZ data) would somehow get corrupted to 0, making incorrect read from undefined areas in the buffer.
(the original code only defined the 0x24 leading bytes to zero and the data did not expect to read data it had not already written)

I had checked that the ripped data matches up to a savestate.

[tvtoon]

LZIterations is unsigned, the break condition is basically undefined.

Write is shorter than the DataBuffer, therefore another undefined behavior.

You must check these boundaries correctly.
***
My mistake, I thought you were subtracting LZiterations twice.

[KingMike]

That is only a snippet of code, LZIterations is a counter value that is set in the full code before it used with a value no bigger than like 35, I usually used unsigned when I am making loop variables (though true, using an actual loop is probably better. Habits from dealing in ROM hex data so much.)
"Write" is an index counter that I initialized with a value of 0x24. (was a duplicate of the long WriteIndex when I wanted to see if WriteIndex being long was the problem, even though I suspected it wasn't and I was missing another real problem.)