Another Sprite Bug (CreateRemoveAlpha)

While coding the alpha mask / alpha channel features for the Paint bounty I was getting some nasty repeatable crashes.

After a lot of digging, I believe I’ve tracked it down to a bug in SpriteExtend SprOp.

The problem is that when using OS_SpriteOp 38 – Create/remove Alpha to add an alpha mask to a sprite, SpriteExtend seems to sometimes write past the end of the sprite area, despite making sure enough space has been reserved (I checked the calculation and even compared the sizes of sprite files with and without alpha masks to make sure).

First, here’s a BASIC program that hopefully illustrates the issue (make sure you increase Next before running):

REM Test Conversion of Sprite Alpha Formats
REM Andy S. November 2021

ONERROR REPORT:PRINT ERL:END
width% = 1024
height% = 768
bpp% = 32
areahdrsize% = 16
sprhdrsize% = 44
masksize% = (((width% + 3) >> 2) << 2) * height%: REM For 1 byte per pixel alpha mask.
REM bytes per row is rounded up to a whole number of words.
bytesperrow% = (((width% * bpp%) + 31) >> 5) << 2
size% = bytesperrow% * height% + areahdrsize% + sprhdrsize% + masksize%
text$ = "After the sprite area."
size2% = size% + LEN(text$) + 1 + 16384: REM 16384 is a safety margin as adding an alpha mask seems to overflow memory
DIM sarea% size2%
sarea%!0 = size%
sarea%!8 = 16
$(sarea% + size%) = text$
SYS "OS_SpriteOp", 256 + 9, sarea%

PRINT "sarea size field:"
PRINT sarea%!0

PRINT "The text at the end of the sprite area is now:"
PRINT $(sarea% + size%)

REM create a new 16M colour BGR sprite with no mask
SYS "OS_SpriteOp", 256 + 15, sarea%, "test", 0, width%, height%, &301680B5

PRINT "After creating the sprite, the text at the end of the sprite area is:"
PRINT $(sarea% + size%)

REM Make sure the original text is there again.
$(sarea% + size%) = "After the sprite area."

REM Add an alpha channel to the sprite
SYS "OS_SpriteOp", 256 + 38, sarea%, "test", &C0000000

PRINT "After adding an alpha channel, the text at the end of the sprite area is:"
PRINT $(sarea% + size%)

REM Make sure the original text is there again.
$(sarea% + size%) = "After the sprite area."

REM convert the alpha channel to an 8-bit alpha mask
SYS "OS_SpriteOp", 256 + 38, sarea%, "test", &80000000

PRINT "After converting to an alpha mask, the text at the end of the sprite area is:"
PRINT $(sarea% + size%)

On my system, the last printed string appears blank, illustrating that the conversion to an alpha mask has overwritten the memory containing the text. Note also that modifying the code to use additional string variables has resulted in “unknown or missing variable” messages when trying to access them and even exceptions, suggesting the sprite has overflowed into the memory BASIC’s using for those variables.

More diagnostic information to follow as well as what I believe is the actual underlying bug.

When I ran this code on a 16M 1024×768 sprite that had an alpha channel, to attempt to convert it to an alpha mask (after ensuring the sprite area has the right amount of space), Paint crashes.

    int space_needed = 0;
    if ((error = os_swix4r (OS_SpriteOp, SpriteReason_CreateRemoveAlpha |
        0x200, *sarea, (int)psprite_address (sprite), flags, NULL, NULL, NULL,
        &space_needed)) != NULL || space_needed > 0 )
    { /*if r3 > 0 it will give amount of extra space needed in area*/
      if (space_needed > 0)
        msg = "More space needed.";
      else
        msg = error->errmess;
      goto finish;
    }

It gets an abort on data transfer at the following location:

*where
Address &FC2912DC is at offset &00004454 in module 'SpriteExtend'
*showregs
Register dump (stored at &2000D2B0) is:
R0  = 000C0000 R1  = 006D5730 R2  = 00255708 R3  = FFFFFFFF
R4  = 0030002C R5  = 0030002C R6  = 00000000 R7  = FC28E6C0
R8  = FFFFFF00 R9  = 00555734 R10 = 00615730 R11 = 00255734
R12 = 2009CEB4 R13 = FA207F70 R14 = FC28E730 R15 = FC2912DC
Mode SVC32 flags set: Nzcvqjggggeaift        PSR = 80000013

It was apparently trying to move the mask memory (actually the memory that will need to follow after the new mask) by R0 = &C0000 = 786,432 bytes, and a 16M 1024×768 alpha mask sprite is 3932204 bytes in memory. With an alpha channel instead it’s 3145772. 3932204 – 3145772 = 786,432, so the value in R0 when it crashed was correct.

The bit that’s obviously wrong in the above register dump is the value in R1, which is past the end of the sprite area (which is &615838).

Here’s the SprExtend code leading up to the crash, with my (not very clear) comments added:

LDR     R10, [R1, #saFree] ; R10's initialised using the existing value of sarea->saFree
ADD     R9, R10, R0 ; Extend area: R9 = saFree + [growth needed in R0, the &C0000 value]
SUBS    R8, R9, R8 ; Check for overflow
DebugIf GT,so,"Not enough room, need:",R8
MOVGT   R3, R8
ADRGTL  R0, ErrorBlock_NotEnoughRoom ; This doesn't happen because there's enough room for the growth needed in R0
ADRGTL  R1, Title
BLGT    copy_error_one
EXIT VS
; Grow area
STR     R9, [R1, #saFree] ; Copies new, larger, saFree offset into sarea->saFree from R9
; Move memory
ADD     R10, R1, R9 ; R10 is initialised to sarea + the newly enlarged sarea->saFree
ADD     R9, R2, R5  ; R9 is the sprite pointer + offset to end of any existing mask (or spNext if no mask)
BL      move_memory_up

Then we branch into the move_memory_up routine inside SprAdjSize:

; --------------------------------------------------------
; Move the memory from [R9 to R10] up to [R9+R0 to R10+R0]
move_memory_up
        Push    "R1,R3,R4,R9,R14"
        ADD     R1, R10, R0 ;    Here's the problem. It adds the offset in R0, &C0000, a second time onto R10
        Debug   id,"Moving memory up from,to,finish:",R10,R1,R9 
        CMP     r9, r10
01
        LDRCC   R3, [R10, #-4]!
        STRCC   R3, [R1, #-4]!
        CMP     r9, r10
        BCC     %BT01
        MOV     lr, #0
        CMP     r9, r1
02
        STRCC   lr, [r1, #-4]!
        CMP     r9, r1
        BCC     %BT02

        Debug   id,"Moved memory up. From,to,finish are now:",R10,R1,R9
        Pull    "R1,R3,R4,R9,PC"

So the bug seems to be that the amount to grow the mask, given in R0, gets added to R10 twice when it should only be done once by move_memory_up.

To prove it, if we subtract the growth amount from R1 twice, we get back the original start of free space in the sprite area:
&6D5734 – &C0000 = &615734
and &615734 – &C0000 = &555734 (the existing sarea free value before the expansion. QED)!

So a bug fix could be to subtract R0 from R9 and R10 immediately before calling move_memory_up.

Thanks very much for fixing this Jeffrey! I ran my BASIC repro against SpriteExtend 1.86 and am happy to confirm it wrote exactly the right number of bytes when converting to each of the mask types, including an extra test I added for on/off masks. :)