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

[Andreas Raab]

Your observation is correct. In some paths, BitBlt performs a prefetch 
for the next input word and it can happen that it reads one word beyond 
the end of the memory used by the form. I found this a couple of years 
ago but didn't have the time to dig into fixing this.

Cheers,
   - Andreas

On 4/4/2012 16:24, Igor Stasenko wrote:
>
> 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.
>