Retroarch: Achievements peeking at memory out of bounds can crash cores/RetroArch

[rzumer]

Description

See https://github.com/LIJI32/SameBoy/issues/162 for background.

Using a build generated prior to defining a memory descriptor that increases the size of the memory map, loading achievements that make use of this memory space causes a crash.

When the memory descriptor is defined, there is no problem.

It is possible that it is related to alt condition sets or some other edge case in the processing where bounds checking was forgotten, as I tested adding such a condition in a core achievement set and saw no crash there (or I was just lucky).

Steps to reproduce the bug

1. Download SameBoy core (currently set to this version), or compile with https://github.com/LIJI32/SameBoy/pull/159 reverted
2. Load "Harry Potter and the Sorcerer's Stone"
3. If achievements are enabled, it should crash on load; otherwise it will run correctly

Version/Commit

• RetroArch: 1.7.3~1.7.6

Environment information

• OS: Windows x86/x64

[orbea]

Have you tried to bisect this? Do you have a backtrace?

[rzumer]

No, and I am not very interested in doing it myself right now unless no one else can reproduce it, especially since there is a new implementation coming up. I don't know how to get a backtrace without recompiling and I don't have a setup to compile. If there is an easy way to get that information please let me know. The log has nothing useful.

[bparker06]

here is the backtrace I get when loading that game (only happened the first time though... can't repro it anymore):

AddressSanitizer:DEADLYSIGNAL
=================================================================
==10989==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000503d (pc 0x555d22ffc48c bp 0x7fffbddd8400 sp 0x7fffbddd83d0 T0)
==10989==The signal is caused by a READ memory access.
    #0 0x555d22ffc48b in cheevos_var_get_value cheevos/var.c:351
    #1 0x555d22fd5f78 in cheevos_test_condition cheevos/cheevos.c:1271
    #2 0x555d22fd6719 in cheevos_test_pause_cond_set cheevos/cheevos.c:1337
    #3 0x555d22fd73c1 in cheevos_test_cond_set cheevos/cheevos.c:1446
    #4 0x555d22fd7dcd in cheevos_test_cheevo cheevos/cheevos.c:1507
    #5 0x555d22fd90ad in cheevos_test_cheevo_set cheevos/cheevos.c:1636
    #6 0x555d22fe0f53 in cheevos_test cheevos/cheevos.c:2474
    #7 0x555d20f6e9b3 in runloop_iterate /home/bp/RetroArch/retroarch.c:4524
    #8 0x555d21351acb in ui_application_qt_run ui/drivers/qt/ui_qt_application.cpp:164
    #9 0x555d20f5305d in rarch_main frontend/frontend.c:172
    #10 0x555d21351c1e in main ui/drivers/qt/ui_qt_application.cpp:188
    #11 0x7f5d93cde222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #12 0x555d20f42ffd in _start (/home/bp/RetroArch/retroarch+0x3e83ffd)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV cheevos/var.c:351 in cheevos_var_get_value
==10989==ABORTING

[rzumer]

Looks like cheevos_var_get_memory() should return a null pointer if var->value >= meminfo.size (this is the behavior on line 271). Then the condition will be ignored at cheevos_var_get_value().

[rzumer]

8587 was not enough.

Error occurred on Monday, April 15, 2019 at 13:57:03.

retroarch.exe caused an Access Violation at location 0000000000942652 in module retroarch.exe Reading from location 000000000000503D.

AddrPC           Params
0000000000942652 000000000985D9A0 0000000000000000 0000000000000000  retroarch.exe!cheevos_var_get_value  [C:/Users/rzumer/Projects/RetroArch/cheevos/var.c @ 360]
   358:          if (memory)
   359:          {
>  360:             value = memory[0];
   361: 
   362:             switch (var->size)
000000000093BADA 000000000985D990 0000000000000005 000000002492E8F0  retroarch.exe!cheevos_test_condition  [C:/Users/rzumer/Projects/RetroArch/cheevos/cheevos.c @ 1271]
  1269:       return 0;
  1270: 
> 1271:    sval          = cheevos_var_get_value(&cond->source) +
  1272:                    cheevos_locals.add_buffer;
  1273:    tval          = cheevos_var_get_value(&cond->target);
000000000093BCD1 0000000025F12630 000000000733E9AC 000000000733E9A8  retroarch.exe!cheevos_test_pause_cond_set  [C:/Users/rzumer/Projects/RetroArch/cheevos/cheevos.c @ 1337]
  1335: 
  1336:       /* always evaluate the condition to ensure delta values get tracked correctly */
> 1337:       cond_valid = cheevos_test_condition(cond);
  1338: 
  1339:       /* if the condition has a target hit count that has already been met,
000000000093BF1C 0000000025F12630 000000000733E9AC 000000000733E9A8  retroarch.exe!cheevos_test_cond_set  [C:/Users/rzumer/Projects/RetroArch/cheevos/cheevos.c @ 1446]
  1444: 
  1445:    /* process the non-Pause conditions to see if the group is true */
> 1446:    return cheevos_test_pause_cond_set(condset, dirty_conds, reset_conds, 0);
  1447: }
  1448: 
000000000093C0DA 000000000A4C6340 000000005DFAAFCC 0000000000000001  retroarch.exe!cheevos_test_cheevo  [C:/Users/rzumer/Projects/RetroArch/cheevos/cheevos.c @ 1507]
  1505:    if (condset < end)
  1506:    {
> 1507:       ret_val = cheevos_test_cond_set(condset, &dirty_conds, &reset_conds);
  1508:       condset++;
  1509:    }
000000000093C4F6 0000000000B06C80 00000000004030CC 0000000000000002  retroarch.exe!cheevos_test_cheevo_set  [C:/Users/rzumer/Projects/RetroArch/cheevos/cheevos.c @ 1636]
  1634:       if (cheevo->active & mode)
  1635:       {
> 1636:          int valid = cheevos_test_cheevo(cheevo);
  1637: 
  1638:          if (cheevo->last)
000000000093E18D 00000000097E9120 0000000000000000 0000000000000000  retroarch.exe!cheevos_test  [C:/Users/rzumer/Projects/RetroArch/cheevos/cheevos.c @ 2474]
  2472:    }
  2473: 
> 2474:    cheevos_test_cheevo_set(&cheevos_locals.core);
  2475: 
  2476:    if (settings)
000000000040AECE 000000000733FD90 0000000000000000 000000000733FD70  retroarch.exe!runloop_iterate  [C:/Users/rzumer/Projects/RetroArch/retroarch.c @ 4524]
  4522: #ifdef HAVE_CHEEVOS
  4523:    if (runloop_check_cheevos())
> 4524:       cheevos_test();
  4525: #endif
  4526:    cheat_manager_apply_retro_cheats();
0000000000401740 0000000000000002 00000000001F0000 0000000000000000  retroarch.exe!rarch_main  [C:/Users/rzumer/Projects/RetroArch/frontend/frontend.c @ 156]
   154:    {
   155:       unsigned sleep_ms = 0;
>  156:       int           ret = runloop_iterate(&sleep_ms);
   157: 
   158:       if (ret == 1 && sleep_ms > 0)
00000000004017A0 0000000000000002 00000000001F0000 0000000007C707B8  retroarch.exe!SDL_main  [C:/Users/rzumer/Projects/RetroArch/frontend/frontend.c @ 184]
   182: int main(int argc, char *argv[])
   183: {
>  184:    return rarch_main(argc, argv, NULL);
   185: }
   186: #endif
0000000000964F0C 0000000000000001 000000000000000A 0000000001732058  retroarch.exe!main_getcmdline
00000000004013A5 0000000000000000 0000000000000000 0000000000000000  retroarch.exe!__tmainCRTStartup  [E:/mingwbuild/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crtexe.c @ 339]
00000000004014DB 0000000000000000 0000000000000000 0000000000000000  retroarch.exe!WinMainCRTStartup  [E:/mingwbuild/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crtexe.c @ 195]
00007FF8F87D3DC4 0000000000000000 0000000000000000 0000000000000000  KERNEL32.DLL!BaseThreadInitThunk
00007FF8F8F13691 0000000000000000 0000000000000000 0000000000000000  ntdll.dll!RtlUserThreadStart

[orbea]

@leiradel Maybe you could help with this issue? Most of us don't really know much about cheevos...

[leiradel]

At first sight it seems the SameBoy memory is not being correctly exposed. There wouldn't be any crashes with official cheevos if it was.

I'll take a look.

[rzumer]

As mentioned in the SameBoy issue linked here, there is a memory descriptor missing from the latest nightly build. When compiled with it, no crash occurs on my end.

RA should avoid crashing due to running out of date cores with achievements though.

On the core's end, returning null memory information (via retro_memory_get_data/size()) until the system and game are initialized doesn't seem to solve the issue.

Returning null memory from cheevos_var_get_memory() in RA works, so the bounds might be incorrect?

[leiradel]

Yes, I agree it shouldn't crash. I'll add the appropriate measures.

[leiradel]

Could someone with everything in place try out this branch?

[rzumer]

I'd already tried changing it that way and it still crashed for me.

returning a null pointer straight from the function fixes the crash but it doesn't seem like the checks are working as expected.

[leiradel]

So the core must be returning bogus information for its memory layout and there's nothing we can do to prevent crashes.

I've pushed another commit but that's only a code simplification. I'll make a PR since the current code is broken, and I suggest SameBoy fixes its memory map.

[leiradel]

All callers of cheevos_var_get_memory check for a NULL return, so the PR is really good except that it can't prevent crashes if the core returns wrong information for the memory map or retro_get_memory_data/size.

[rzumer]

SameBoy no longer crashes following recent updates, so unless I made a mistake in earlier tests I believe @leiradel was correct and incorrect RAM size reported from the core's side was leading to the crash. As mentioned, this is not really avoidable on RA's end, so closing as resolved.