[hatari-devel] Hatari crashes with Mental Hangover demo

[Nicolas Pomarède]

Le 30/04/2011 18:46, Eero Tamminen a écrit :
> Hi,
>
>
> 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.

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 ];
}


> 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.

>
> 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.

Nicolas


>
> 	- Eero
>
> 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 ?