TLN_SetRenderTarget with a larger pitch causes heap buffer overflow (segfault)

[alvinhochun]

For example, if we do the following:

TLN_Init(240, 160, 4, 128, 0);
TLN_SetRenderTarget(framebuffer, 4 * 256); // texture width must be power-of-2

Then when calling TLN_UpdateFrame we get a heap buffer overflow at:

Tilengine/src/Draw.c

Line 173 in d4f8737

memset(engine->priority, 0, engine->framebuffer.pitch);

The buffer is allocated at:

Tilengine/src/Tilengine.c

Line 106 in d4f8737

context->priority = (uint32_t*)malloc(context->framebuffer.pitch);

[megamarc]

• The framebuffer must be big enough to store a full frame, at 4 bytes/pixel (32 bpp). That is: w*h*4.
• The pitch is the number of bytes that each scanline takes. That is, w*4

uint8_t framebuffer[240 * 160 * 4];

TLN_Init(240, 160, 4, 128, 0);
TLN_SetRenderTarget(framebuffer, 240 * 4);

BTW, texture width is not required to be power of 2

[alvinhochun]

texture width is not required to be power of 2

It could be required on certain GPU architectures. The point is, it sometimes makes sense to want to have Tilengine draw each row of pixels with a "pitch" (also called "stride") of some specific alignment for various reasons, otherwise the user may have to memmove/memcpy each row to fix that.

Is the pitch argument not intended for this purpose? I mean, if it is supposed to be w * 4 then I don't see the point of having it as an argument.

[megamarc]

There isn't any problem in setting a pitch bigger that the required by the framebuffer (for example, setting 320x240 inside a texture of 512x256). Just make sure you're allocating the framebuffer correctly with required memory. I've tested right now and it works as expected.

[alvinhochun]

I've tested right now and it works as expected.

Please test with ASan (-sanitize=address). With the code I am testing it shows a heap-buffer-overflow at the memset call I mentioned above.

Looking at the code:

• When the buffer context->priority = (uint32_t*)malloc(context->framebuffer.pitch); is allocated, framebuffer.pitch is set to width * 4.
• If TLN_SetRenderTarget is called with a larger pitch afterwards, framebuffer.pitch is set to that larger value.
• Finally memset(engine->priority, 0, engine->framebuffer.pitch); this causes a buffer overriun because framebuffer.pitch is now larger than the allocation (width * 4).