[Scummvm-devel] Re: CVS: scummvm/scumm akos.cpp,1.102,1.103 script_v6.cpp,1.271,1.272

Max Horn max at quendi.de
Fri Jan 16 04:41:01 CET 2004


Am 16.01.2004 um 10:02 schrieb Travis Howell:

> Max Horn wrote:
>> Am 16.01.2004 um 07:16 schrieb Travis Howell:
>>
>>> Update of /cvsroot/scummvm/scummvm/scumm
>>> In directory sc8-pr-cvs1:/tmp/cvs-serv19453/scumm
>>>
>>> Modified Files:
>>> akos.cpp script_v6.cpp
>>> Log Message:
>>>
>>> Checks were a bit too strict, makes fatty bear completable.
>> [...]
>>> Index: script_v6.cpp
>>> ===================================================================
>>> RCS file: /cvsroot/scummvm/scummvm/scumm/script_v6.cpp,v
>>> retrieving revision 1.271
>>> retrieving revision 1.272
>>> diff -u -d -r1.271 -r1.272
>>> --- script_v6.cpp 16 Jan 2004 04:55:49 -0000 1.271
>>> +++ script_v6.cpp 16 Jan 2004 06:16:38 -0000 1.272
>>> @@ -411,7 +411,7 @@
>>>  // FIX THE FIXME: fixing an assert by commenting out is bad. It's
>>> evil.
>>>  // It's wrong. Find the proper cause, or at least, silently return
>>>  // from the function, but don't just go on overwriting memory!
>>> - assert(base >= 0 && base < ah->dim1_size * ah->dim2_size);
>>> + assert(base >= 0 && base <= ah->dim1_size * ah->dim2_size);
>>>
>>>  if (ah->type == 4) {
>>>  return ah->data[base];
>>> @@ -428,7 +428,7 @@
>>>  return;
>>>  base += idx * ah->dim1_size;
>>>
>>> - assert(base >= 0 && base < ah->dim1_size * ah->dim2_size);
>>> + assert(base >= 0 && base <= ah->dim1_size * ah->dim2_size);
>>>
>>
>> I don't see why that change should be valid... after all the arrays do
>> contain exactly dim1_size* dim2_size entries. The case you added to
>> the assert exceeds that, so to me it looks as if it would write to
>> invalid memory (or read from it).
>>
>> Maybe the change *is* correct, but then it can't go alone w/o also
>> changing at the very least defineArray() (which then must allocate
>> more memory).
>>
>> However, I find this a bit strange. Right now, we interpret
>> readArray(array, idx, base) as doing (in pseudo code):
>>    array[idx][base]
>> but convert this 2-dim access to 1-dim by
>>    array[idx*dim2 + base]
>> idx would properly range from 0 to dim1-1, and base from 0 to dim2-1.
>> If your change above is really required, then this assumption is
>> totally wrong and we have been storing arrays (at least for HE games)
>> incorrectly. Unless I am mistaken, of course, which is very well
>> possible at 8 AM w/o any breakfast, so anybody please correct me where
>> necessary.
>
> The change was just to get 'Fatty Bear's Birthday Surprise' 
> completable for
> now, I checked disasm. from 'Putt-Putt Joins The Parade' but I'm not 
> sure.

See, that's irrelevant. Right now, we are potentially reading 
from/writing to invalid memory, period. Either revert that change, or 
modify defineArray to allocate a bigger array... Also, you might want 
to document how to trigger the "old" assert so that people can check 
the scripts (i.e. whether this is a scripting bug in FB, or indeed an 
engine difference, or if our array implemention simply was wrong all 
the time, in which case we'd have a big problem...


> I have attached writeArray dissasm. from 'Putt-Putt Joins The Parade' 
> if you
> want to check.
>
I will when I am back on sunday night.


Cheers,

Max





More information about the Scummvm-devel mailing list