Re: [hatari-devel] memory setup segfault

[Laurent Sallafranque <laurent.sallafranque@xxxxxxx>]

Hi all,


I've just tested the path and I still have a core dump when I switch 
from Falcon to ST.

I'll give you mode context soon.

Regards


Le 04/11/2023 à 23:08, Nicolas Pomarède a écrit :
> Le 23/09/2023 à 22:32, Nicolas Pomarède a écrit :
> > Le 23/09/2023 à 19:30, Eero Tamminen a écrit :
> > > Hi Nicolas,
> > >
> > > This is not logging issue, but Hatari memory setup one.
> > >
> > > Over ~143 KB area of Hatari process memory, including its global 
> > > variables, was overwritten with value 0x81, and preceding memory 
> > > area with value 0x48.
> > >
> > > Among those global variables, is FILE pointer to Hatari log file.  
> > > Both in my and Laurent's case, Hatari segfaulted when fputs() tried 
> > > to use the pointer that had been changed to invalid 
> > > 0x8181818181818181 address.   It's a bit miracle Hatari got that far...
> > >
> > > ASAN reports memory setup to be the culprit:
> > > ------------------------------------------
> > > ==10734==ERROR: AddressSanitizer: global-buffer-overflow on address 
> > > 0x5625d1dd6e20 at pc 0x5625ce3f0b68 bp 0x7ffc142ee490 sp 0x7ffc142ee488
> > > WRITE of size 1 at 0x5625d1dd6e20 thread T0
> > >      #0 0x5625ce3f0b67 in map_banks2 src/cpu/memory.c:1959
> > >      #1 0x5625ce3f0b67 in map_banks_ce src/cpu/memory.c:1988
> > >      #2 0x5625ce3f0b67 in memory_map_Standard_RAM src/cpu/memory.c:1614
> > >      #3 0x5625ce3f0f79 in memory_init src/cpu/memory.c:1748
> > >      #4 0x5625ce2e9d4c in TOS_InitImage src/tos.c:1132
> > >      #5 0x5625ce2c10f1 in Reset_ST src/reset.c:61
> > >      #6 0x5625ce23050b in Change_CopyChangedParamsToConfiguration 
> > > src/change.c:504
> > >      #7 0x5625ce236b17 in Dialog_DoProperty src/dialog.c:70
> > >      #8 0x5625ce2de904 in ShortCut_ActKey src/shortcut.c:297
> > >
> > > 0x5625d1dd6e20 is located 32 bytes to the left of global variable 
> > > 'TTmem_mask' defined in 'src/cpu/memory.c:41:16' (0x5625d1dd6e40) of 
> > > size 4
> > > 0x5625d1dd6e20 is located 0 bytes to the right of global variable 
> > > 'ce_banktype' defined in 'src/cpu/memory.c:114:8' (0x5625d1dc6e20) 
> > > of size 65536
> > > ------------------------------------------
> > >
> > > I could not reproduce this by switching from TT emulation to ST one, 
> > > or with EmuTOS, only with real TOS.
> > >
> > >
> > > Smallest run-time change triggering the issue following...
> > >
> > > 1. Start Hatari with Falcon TOS and >4MB RAM:
> > > hatari --machine falcon --tos tos404.img -s 8
> > >
> > > 2. Switch to 1.x / 2.x TOS + 4MB or less RAM in Hatari setup dialog
> > >
> > > 3. OK changes, so that emulation gets rebooted
> >
> > Hi
> >
> > thanks for the trace, will have a look later.
> >  From the corresponding source code, it might be possible that 
> > MMU_Bank0_Size comes with a too big value in that case, not 
> > compatible with STF memory, thus overriding some memory regions.
>
> Hi
>
> took more time to fix due to not many spare time recently.
>
> Anyway, it should now be OK. The problem was that in Falcon mode 
> MMU_Bank0_Size is not used and is set to 0 at some point.
>
> But after changing tos to 1.x/2.x, this value of 0 was kept, which 
> caused a problem in memory_init() and memory_map_Standard_RAM() :
>
> map_banks_ce(&STmem_bank_MMU, 0x10000 >> 16, ( MMU_Bank0_Size - 
> 0x10000 ) >> 16, [...]
>
> Ooops, MMU_Bank0_Size==0  would mean map_banks_ce will in fact use a 
> huge/wrong value (-0x10000 as unsigned) instead of the correct bank's 
> size.
>
> I added a call to Memory_Reset(true) when a change to TOS version 
> implies a change to the machine's type.
>
> But I think tos.c has too much specific cases trying to "fix" 
> machine/hardware to match the tos version. Instead of doing some 
> UnInit/Init for various components it would be safer to call 
> Change_DoNeedReset() (as when changing parameters from the UI) and 
> then call Reset_ST(true) to force a cold reset to be sure everything 
> start with sane default values.
>
> Nicolas