Interrupt-induced race condition in math32 lib for sdcc

[vmorilla]

Hi,

This corresponds to a pull-request I have just made with the solution (https://github.com/z88dk/z88dk/pull/2714), but wanted to share it with you in case it can help someone in the future. This bug was a nasty one and has been hunting me for days. In short, this is a race condition with interrupts in the sdcc math32 library. Since sdcc passed 8 bits values as just 1 octect in the stack, it is common to see the following pattern:

Code: Select all

	pop	bc	;return value
	dec	sp
	pop	af	;1 octect value goes to a
        push bc
        ...

However, in cm32_sdcc___uchar2fs_callee.asm we had this:

Code: Select all

	pop	bc	;return
	pop	hl	;value goes to L
	dec	sp
	push	bc
	ld	h,0
        ...

Apparently harmful and faster since the 1 byte value is stored directly in L... However, if there is an interruption right after the second pop, the content of the stack will be overwritten. So, it is very important that the stack is decremented before the pop, so that the content of the stack is never left vulnerable to interruptions:

Code: Select all

	pop	bc	;return
	dec	sp
	pop	hl	;value goes to H now, but stack is not exposed to undesired writes from an interruption
	push	bc
        ld  l,h
	ld  h,0

I wonder if there are similar bugs in the code... I see how easy is to introduce bugs like this. I also wonder why on earth sdcc has introduced this complexity. Does it really make a difference to save 1 byte in the stack?

[vmorilla]

BTW, I came accross this a part of the development of my new video-game for the Spectrum Next: Next Point. You can download it here: https://vmorilla.github.io/next-point/. I have to say I am quite happy with the results and I am receiving very good feedback, so I wanted to take the chance to thank all z88dk contributors, and DOM specially, for making this possible.

[feilipu]

Glad you wrote this up here. It is a very subtle bug. Congratulations on identifying and capturing it.


I’m (we’re) going through the whole development kit now to find other cases where this could happen. Hopefully it will improve reliability for RTOS operations in particular.

[Timmy]

Thank you for discovering, that sounds like a very nasty bug.

I had to re-read several times until this sinks in, it's very unintuitive.

I personally never do that kind of thing myself, but now I will also try to avoid it in the future. Thanks!