[Vm-dev] There might be a bug in Bitblt copyloop

[Igor Stasenko]

The way how i discovered  it is a bit complicated, so i need to
explain it in detail.

I am using a SurfacePlugin for gluing the cairo image surface (which
is holds a bitmap bits somewhere in C heap) with bitblt.

For that, you need to register a surface with surface plugin and
provide a callbacks by filling following structure:

typedef struct sqSurfaceDispatch {
	/* Version information. Must be provided by the client
	   so the surface manager can check if certain operations
	   are supported. */
	int majorVersion;
	int minorVersion;

	/* Version 1.0 */
	fn_getSurfaceFormat getSurfaceFormat;
	fn_lockSurface lockSurface;
	fn_unlockSurface unlockSurface;
	fn_showSurface showSurface;
} sqSurfaceDispatch;


The function to register a surface is provided by a surface plugin is following:

	int ioRegisterSurface(int surfaceHandle, sqSurfaceDispatch *fn, int
*surfaceID);
		Register a new surface with the given handle and
		the set of surface functions. The new ID is returned
		in surfaceID. Returns true if successful, false
		otherwise.

you passing a handle (which in my case is cairo surface handle, the
filled struct and as result you get a surfaceID.

A surface ID is one which is used directly in language side, by
setting a Form's bits instance variable to smallinteger with id value.

I.e. imagine that i call ioRegisterSurface:

int id = 0;
 ioRegisterSurface(myHandle, myDispatch, &id);

and then in language side i creating a Form with same id:

externalForm := Form extent: (self width at self height) depth: 32 bits: id .

Now you can use this form for any bitblt operations.. and when it
happens, a bitblt detects that bits are not an instance of Bitmap,
but a smallinteger.. and so instead of working with bitmap bits
directly it using a sqSurfaceDispatch structure, which we provided
before
to access surface bits.

So, the most important one is getSurfaceFormat function which having
following form:

int getSurfaceFormat(int handle, int* width, int* height, int* depth,
int* isMSB);
		Return general information about the OS drawing surface.
		Return true if successful, false otherwise.

		The returned values describe the basic properties such as
		width, height, depth and LSB vs. MSB pixels.


And now finally about the bug itself.
I implemented all these functions (needed for sqSurfaceDispatch) using
nativeboost and they worked well, but for
surfaces of certain dimensions i got VM crashes.

For instance, if i create a surface 400 at 400 x 32bpp it works perfectly,
but if i create a surface 800 at 800 x 32bpp it crashing..
however if i use
801 at 801, it works..

after some debugging i discovered that bitblt tries to read bytes past
the actual buffer size, i.e.
if in getSurfaceFormat() i reporting
width=800
height= 800
depth= 32
msb = true

i see that bitblt reading bytes past
buffer start address + (800*800*4) bytes

(there is additional function involved, which answers the pitch for
the scanline:
int lockSurface(int handle, int *pitch, int x, int y, int w, int h);

so it is actually not (800*800*4)
but

pitch * (height = 800)

anyways...
i placed a workaround by reporting 1 pixel less height, so if i have 800*800
bitmap, i reporting 800*799

Why we don't have issues with this in image? Well i think because you
can safely read couple more bytes past any object in object memory
and be 100% sure that you won't have SEGFAULT.
In contrast, if you working with buffers allocated on C heap, and your
buffer size is close to page size alignment (4kb) it could be quite
easy to
hit SEGFAULT by reading even 1 byte past the buffer size.

So, i suspect there is a bug in one of the copy loop bounds, something
as subtle as:

[0 ... count]
vs
[0 .. count)

i.e. it loops over
0 to: count
instead of:
0 to: count -1

i would appreciate if people with better expertise about bitblt will
comment what i have found.

--
Best regards,
Igor Stasenko.