[Bf-committers] "off by one" error in renderdatabase.c?

[Martin Dickopp]

Ton Roosendaal <ton at blender.org> writes:

>> I think I know what is causing this. By looking
>> how the function RE_freeRotateBlenderScene in
>> source/blender/renderconverter/intern/convertBlenderScene.c
>> frees the elements of the arrays R.blove, R.blovl and R.bloha,
>> I conclude that these arrays are supposed to be terminated by
>> an element which is a null pointer.
>
> No that is not true. The arrays are initialized with NULL pointers,
> and then all filled with data.

Hi,

There is the following code in RE_freeRotateBlenderScene:

        a=0;
        while(R.blove[a]) {
                MEM_freeN(R.blove[a]);
                R.blove[a]=0;
                a++;
        }
        a=0;
        while(R.blovl[a]) {
                MEM_freeN(R.blovl[a]);
                R.blovl[a]=0;
                a++;
        }
        a=0;
        while(R.bloha[a]) {
                MEM_freeN(R.bloha[a]);
                R.bloha[a]=0;
                a++;
        }

This code frees all pointers in the arrays until it encounters a null
pointer. If the arrays are not supposed to be null-terminated, then
the above loops need a different termination condition.

When I render my sample scene, R.blovl[0] to R.blovl[4095] are valid
pointers, while R.blovl[4096] has the value 0x214b434f, or "OCK!" in
ASCII, i.e. the "memory area end guard" from
intern/guardedalloc/intern/mallocn.c. This causes the "attempt to free
illegal pointer" message.

> Your proposed patch just skips the last free entry...

My proposed patch makes sure that additional memory is allocated one
step earlier. With the original code, the usage of the array evolves
like this while it is filled:

   ...
   allocated 0 to 4095, used 0 to 4093, zero-filled 4094 to 4095
   allocated 0 to 4095, used 0 to 4094, zero-filled 4095
   allocated 0 to 4095, used 0 to 4095, nothing zero-filled
   additional allocation
   allocated 0 to 5119, used 0 to 4096, zero-filled 4097 to 5119
   ...

My proposed patch changes it to:

   ...
   allocated 0 to 4095, used 0 to 4093, zero-filled 4094 to 4095
   allocated 0 to 4095, used 0 to 4094, zero-filled 4095
   additional allocation
   allocated 0 to 5119, used 0 to 4095, zero-filled 4096 to 5119
   allocated 0 to 5119, used 0 to 4096, zero-filled 4097 to 5119
   ...

> why would that fix your issue,

It avoids the step indicated with "nothing zero-filled" above, therefore
making sure that the arrays are null-terminated at all times.

> and why only for 64 bits?

That I don't know. But since the exact behavior depends on what happens
to be in memory after the arrays, I could image different outcomes on
different architectures.

Am I correct to assume that how R.blovl is used depends only on the
scene to be rendered, and not on the architecture of the machine?

I will try to come up with a testcase for 32 bit, but my only 32 bit
machine is an old laptop with 128 MB RAM. It takes 7 hours (mostly of
memory swapping) to render my sample scene, so I cannot work very
efficiently on this machine. :)

Thanks,
Martin