[hatari-devel] Hatari crashes with Mental Hangover demo

Eero Tamminen eerot at users.berlios.de
Sat Apr 30 20:16:49 CEST 2011


Hi,

On lauantai 30 huhtikuu 2011, Nicolas Pomarède wrote:
> Le 30/04/2011 18:46, Eero Tamminen a écrit :
>> When looking at the fdc.c, it obviously doesn't do any address
>> validation before scribling over Hatari process memory with "a random"
>> offset it got from the emulated Atari program.
> 
> Yes, I saw that at that time, when I looked for what was causing the
> boot sector to misbehave.
> 
> > However, I'm not sure whether the right thing is to mask the FDC DMA
> > address to a valid address, just ignore requests with invalid address
> > or raise an exception?
> 
> I think the right thing is to call STMemory_ValidArea. In the case where
> memory region is not valid correct fix would be to ensure "addr" is
> masked to 24 bits and if one part of the region from "addr" to "addr +
> len" is not a valid region, then each address should be masked to not be
> outside of the ram.
> 
> In that rare case, I think it would be better to use a "for" loop
> instead of memcpy.

I was more of wondering whether the registers were now handled wrong and 
should they be somehow be masked to be handled correctly according to some
documentation. That way the change would be isolated to single function.

Compedium didn't mention anything.


> For example sthg like :
> 
> for ( i=0 ; i<nRetLen ; i++ )
> {
>    dest = nDmaAddr+i;
>    dest = dest % CURRENT_SIZE_OF_RAM
>    (char *)STRam[dest] = (char *)retbuf[ i ];
> }

Is that really better than just ignoring the read completely when any of its
area is outside of Atari RAM? :)


> > The attached patch logs&  ignores invalid FDC DMA addresses used in
> > fdc.c and hdc.c.  This fixes the Hatari crash, but the demo doesn't
> > work so I guess the address should be masked instead...?
> 
> That's not related to masking the address ; have you run the demo
> without enabling the RTC and without HD emulation as I wrote in another
> thread ? Unless you do that, the demo will never boot, the boot sector
> is buggy.

Ah, right I forgot that.  So just ignoring invalid requests seems fine?

(The good thing about vacation is that one forgets things, especially
work related stuff, but also some other stuff. ;-))


> > Btw. Second patch does some code cleanup by removing from fdc.h header
> > anything that doesn't doesn't need to be there + sets functions only
> > used within fdc.c as static.  Is it OK to commit it?
> 
> Seems OK for me.

Ok, I'll commit it after stuff below is clear.


> > PS. While doing the patches, I noticed:
> > $ grep HDCSectorCount $(find . -type f)
> > ./hdc.h:extern short int HDCSectorCount;
> > ./hdc.c:short int HDCSectorCount;
> > ./hdc.c:       fprintf(hdlogFile, "HDC sector count: 0x%x\n",
> > HDCSectorCount);
> > ./fdc.c:       HDCSectorCount = 0;
> > ./fdc.c:               DMAStatus_ff8606rd |= (HDCSectorCount)?0x2:0;
> > /* HDC */
> > ./fdc.c:               DiskControllerByte = HDCSectorCount;
> > 
> > What's the point in sector count that's never non-zero?
> 
> What do you mean exactly ?

From above you can see that "HDCSectorCount" can never get any other value
than zero (unlike e.g. FDCSectorCountRegister which seems to be used for
similar purpose).  So what's the point in having that variable?

hdc.c code gets the HDC sector count with this macro, it doesn't use
the variable:
#define HD_SECTORCOUNT(a) (a.command[4] & 0xFF)        /* get sector count 
*/

How the fdc.c & hdc.c interact is quite ugly as fdc.c uses hdc.c variables
directly instead of there being HDC_* functions for this stuff:
------------
static void FDC_ResetDMAStatus(void)
{
...
        /* Reset HDC command status */
        HDCSectorCount = 0;
        /*HDCCommand.byteCount = 0;*/  /* Not done on real ST? */
        HDCCommand.returnCode = 0;
}

void FDC_SetDMAStatus(bool bError)
{
...
        /* Set zero sector count */
        if (DMAModeControl_ff8606wr&0x08)         /* Get which sector count? 
*/
                DMAStatus_ff8606rd |= (HDCSectorCount)?0x2:0;         /* HDC 
*/
        else
                DMAStatus_ff8606rd |= (FDCSectorCountRegister)?0x2:0; /* FDC 
*/
        /* Perhaps the DRQ should be set here */
}

void FDC_DiskControllerStatus_ReadWord(void)
{
        Sint16 DiskControllerByte = 0;            /* Used to pass back the 
parameter */
...
        if ((DMAModeControl_ff8606wr & 0x18) == 0x08)     /* HDC status reg 
selected? */
        {
                /* return the HDC status reg */
                DiskControllerByte = HDCCommand.returnCode;
        }
        else if ((DMAModeControl_ff8606wr & 0x18) == 0x18)  /* HDC sector 
counter??? */
        {
                Log_Printf(LOG_DEBUG, "*** Read HDC sector counter???\n");
                DiskControllerByte = HDCSectorCount;
        }
        else
-----------


	- Eero



More information about the hatari-devel mailing list