Re: [hatari-devel] EKO System crash

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 31/10/2020 à 09:25, Thomas Huth a écrit :
> Am Sat, 31 Oct 2020 09:08:04 +0100
> schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:
>
> > Le 31/10/2020 à 07:13, Thomas Huth a écrit :
> > > Am Thu, 29 Oct 2020 00:11:52 +0200
> > > schrieb Eero Tamminen <oak@xxxxxxxxxxxxxx>:
> > >    
> > > > On 10/27/20 1:12 AM, Eero Tamminen wrote:
> > > [...]
> > > > > > EKO Systems:  perfectly works (except a little transition in the
> > > > > > betty boo image).
> > > > > >          Music plays well until the 3D scifi scene and plays well
> > > > > > again in the tridi world.
> > > >
> > > > Close to end I got Hatari crash:
> > > > -----------------------------------
> > > > WARN : crossbar DMA Play: Illegal buffer size (from 0x0d84e8 to
> > > > 0x0d84bc) Segmentation fault
> > >
> > > Looking at the crossbar code, I noticed that this could happily read
> > > from beyond the STRam[] array if a Falcon program like EKO system
> > > set the DMA start address higher than the end address.
> > >
> > > I've changed to code a little bit so that this should hopefully not
> > > happen anymore, i.e. I hope it also won't crash anymore (though I
> > > was not able to reproduce the crash here).
> > >
> > > As a bonus, the music should now play in the 3D racing scene of the
> > > demo.
> > >
> > > Note: I've also updated the recording function accordingly, but I
> > > did not test that part. Laurent, could you please try whether AFM
> > > still works fine for you? Thanks!
> >
> > Hi
> >
> > I see you're doing "% STRamEnd" to limit the range ; maybe it would
> > be better to use "&" to mask out of limit bits instead, as this is
> > more "hardware like" ?
>
> Which mask do you suggest? 0x00ffffff? But then we definitely still need
> an additional check to make sure that we do not go beyond STRamEnd
> here, to avoid that we access memory beyond the end of STRam[]...
>
> BTW, how's that situation handled in src/dmaSnd.c ? I did not spot any
> sanity checks there at a very first glance?

Hi

for DMA sound in STE mode, mask is computed using DMA_MaskAddressHigh() 
which will return 0x3f in ST/STE mode because sound (or video counter 
too) is restricted to 4 MB.

Now, looking at dmaSnd.c, I see it also accesses STRam[] directly which 
is an error I think.

Considering this is DMA, we should probably use sthg similar to what I 
used in blitter.c Blitter_ReadWord() :

        /* When reading from a bus error region, just return a constant */
        if ( STMemory_CheckAddrBusError ( addr ) )
                value = BLITTER_READ_WORD_BUS_ERR;
        else
                value = (Uint16)get_word ( addr );

So, instead of accessing STRam, we call get_word() if it's not a bus 
error region and this will call the corresponding memory handler for the 
memory region. This way, we never cross the boundaries of STRam.

dmaSnd.c is also accessing STRam, I know that the demo "amberstar 
cracktro" is buggy and will set start=end, which will play through the 
whole memory, but it doesn't crash Hatari, maybe by luck and because the 
mask 0x3f restrict it to a smaller region.

So, dmaSnd.c should also be modified to use get_word().

We could add a common function DMA_ReadWord() in m68000.c to be called 
by blitter.c / dmaSnd.c / crossbar.c.

What do you think ?

Nicolas