Re: [hatari-devel] memory setup segfault (was: logging segfault)

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

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