[hatari-devel] video.c happily overwriting Hatari memory and crashing (when user uses border pixel values < 16)

npomarede at corp.free.fr npomarede at corp.free.fr
Thu Mar 18 00:14:49 CET 2010


On Wed, 17 Mar 2010, Eero Tamminen wrote:

> Hi,
>
> I was getting this crash in video.c on Falcon emulation after applying
> the BorderPixels patch (from another mail which affects ST/e borders
> handling):
> --------
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0xb79c96b0 (LWP 4444)]
> 0xb7c67727 in memset () from /lib/i686/cmov/libc.so.6
> (gdb) bt
> #0  0xb7c67727 in memset () from /lib/i686/cmov/libc.so.6
> #1  0x0808001f in Video_InterruptHandler_HBL () at video.c:1862
> #2  0x08087d39 in m68k_go (may_quit=1) at newcpu.c:1754
> #3  0x0806bf04 in M68000_Start () at m68000.c:228
> ---------
>
> Crash happens on this line:
>                        memset(pSTScreen,0,SCREENBYTES_LEFT-2);
>
> includes/screen.h:
> #define SCREENBYTES_LEFT    (nBorderPixelsLeft/2)  /* Bytes for left border
> */
>
>
> After reading the code, I think the issue comes from the patch zeroing
> nBorderPixels* screen.c variables when borders aren't enabled and memset()
> getting -2 as count as result.   It's not a bug in the patch, the patch just
> reveals an existing bug in video.c.
>
>
> In the current code nBorderPixels* variables values come from configuration
> values and are truncated down to something divisable by 16.  If user happens
> to set this to 15 or smaller, Hatari can crash.
>
> Even worse is this in video.c:
> memset ( pSTScreen, 0, SCREENBYTES_LEFT-BORDERBYTES_LEFT_2_STE+4 );
> memcpy ( pSTScreen+SCREENBYTES_LEFT-BORDERBYTES_LEFT_2_STE+4,
> pVideoRaster+VideoOffset+4, BORDERBYTES_LEFT_2_STE-4 );
>
> Or:
> memset(pSTScreen,0,SCREENBYTES_LEFT-4*2);
> memcpy(pSTScreen+SCREENBYTES_LEFT-4*2, pVideoRaster, 4*2);
>
> These will result in video.c overwriting 16 bytes before pSTScreen.
>
> Needs to be fixed before release...
>

Well, the code is clearly "designed" that way, having border a multiple of 
sthg else than 16 would not have a real meaning, because we must have a 
number of bytes that matches 4 planes in lowres.

What should we do with 15 ? Add a "black" bytes to go to the closest 16 
bytes multiple ? This would be quite tricky to handle and would create a 
lot of cases in video.c (and this part is already quite complicated).

I think the gui should not allow to set a non multiple of 16 for the 
border when in st/ste mode, handling that in video.c would create too much 
problem (from what I understood, changing borders was mainly asked by 
users running in falcon mode, not in ste/stf)


Nicolas




More information about the hatari-devel mailing list