search

rtissera / libchdr

Standalone library for reading MAME's CHDv1-v5 formats.
BSD 3-Clause "New" or "Revised" License
98 stars 42 forks source link

[Crash] Last changes broke .BIN/.IMG/.CSO file support #82

Closed DrUm78 closed 1 year ago

DrUm78 commented 2 years ago

Software: PCSX fork using libchdr: https://github.com/DrUm78/pcsx_rearmed Hardware: FunKey S handheld device with armv7 NEON

Not sure what commit broke it but PS1 ROMs with .bin or .img format do not load anymore, the user is just sent back to the desktop. Nothing relevant in the logs I got from GMenu2X. That works with https://github.com/rtissera/libchdr/commit/045f2a7f23f5ac10c0122f6f56e42023dfb71e2a but not with last https://github.com/rtissera/libchdr/commit/9882eeaaa2fc29dac3184056a92e8227f583e2e6. .PBP and .CHD still work however. Interesting thing: last commit works well with .bin/.img on PicoDrive emulator on the same platform but breaks .cso file format too.

Steps:

  1. Try to load any ROM with .cue/.bin or .cue/.img format
  2. Notice that you are sent back to the desktop
DrUm78 commented 2 years ago

@rtissera The commit that breaks it is https://github.com/rtissera/libchdr/commit/40ec221c8a35a2fb90dc640a1e4e9fd6524506e8.

snickerbockers commented 2 years ago

I'm a bit confused at the connection between .bin, .img and .PDP being broken as a result of a change to libchdr. In theory that shouldn't be possible because this library does not implement those rom formats.

Are you sure this isn't a pre-existing memory corruption? Commit 40ec221c8a35a2fb90dc640a1e4e9fd6524506e8 did make major changes to the chd_core_file typedef; adding or removing members to or from a struct is often all it takes to cause the effects of a previously-latent stack or heap corruption to manifest.

Interesting thing: last commit works well with .bin/.img on PicoDrive emulator on the same platform but breaks .cso file format too.

is this referring to a specific fork of picodrive? I don't see libchdr referenced anywhere in the upstream repo. Also same confusion about .cso not being a format that libchdr implements.

rtissera commented 2 years ago

@snickerbockers agree @DrUm78 I'm not claiming the VFS commit you're mentioning could not be the culprit of your issues, but except if we introduced inside this commit some heap/stack corruption, I would rather think you're facing a bug in your code which did not popped before.

If you could build your code with full debugging, launch through gdb and get some crash info/stack, maybe we could determine if libchdr is linked to your issue or not.

DrUm78 commented 2 years ago

@snickerbockers Yeah I know that libchdr should not break .bin/.img and .cso but bumping to https://github.com/rtissera/libchdr/commit/40ec221c8a35a2fb90dc640a1e4e9fd6524506e8 or above really breaks their support for PCSXR and PicoDrive though, don't know why. About PicoDrive, the fork is https://github.com/irixxxx/picodrive, which is much more advanced than notaz' one and includes all the latest code from irixxxx.

@rtissera I should have run gdb before with a non stripped binary and -g option, seems to be a segfault, here are the logs for PicoDrive:

Program received signal SIGSEGV, Segmentation fault.
0xb6f74f6c in free () from /lib/ld-musl-armhf.so.1
#0  0xb6f74f6c in free () from /lib/ld-musl-armhf.so.1
#1  0xb6f95ca4 in fclose () from /lib/ld-musl-armhf.so.1
#2  0xb6ad5070 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
A debugging session is active.
    Inferior 1 [process 1346] will be killed.
Quit anyway? (y or n) [answered Y; input not from terminal]
snickerbockers commented 2 years ago

@DrUm78 can you please try pointing your git submodule to my fork at https://github.com/snickerbockers/libchdr/tree/chd_read_header_uninit_fix and see if that fixes anything?

IDK if this will actually solve anything but i stumbled into it while building your PCSX fork and it can theoretically cause a bad call to fclose so maybe that'll fix it. I'm unable to test it myself because qBittorrent hasn't finished downloading my "legal backup copy" of all these PSX games.

DrUm78 commented 2 years ago

@snickerbockers I applied this commit https://github.com/snickerbockers/libchdr/commit/0d227d45d03e6a6716065463a2a0549dc2276d45 on top of the updated libchdr branch but that does not fix it unfortunately. The stack is just a bit different:

Program received signal SIGSEGV, Segmentation fault.
0xb6ecbf6c in free () from /lib/ld-musl-armhf.so.1
#0  0xb6ecbf6c in free () from /lib/ld-musl-armhf.so.1
#1  0xb6eecca4 in fclose () from /lib/ld-musl-armhf.so.1
#2  0xb6a2c070 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
A debugging session is active.
    Inferior 1 [process 5307] will be killed.
Quit anyway? (y or n) [answered Y; input not from terminal]
DrUm78 commented 2 years ago

@snickerbockers If you want to test then build the original repo instead (mine has some very specific changes for the FunKey S handheld): https://github.com/notaz/pcsx_rearmed I built notaz' for Linux and I have the same crash with .bin/.img with the faulty commit while everything works fine before that commit.

rtissera commented 2 years ago

@DrUm78 building with glibc or musl ? I noticed you use musl in your backtraces (which should be fine, I'm a musl big fan myself), but sometimes it can have subtle behavior difference with glibc.

DrUm78 commented 2 years ago

@rtissera I use musl when I compile it for the FunKey S device yes but glibc when compiling for Linux x86_64 (Ubuntu 20.04) and the same happens. Can you reproduce the issue yourself when compiling for Linux?

DrUm78 commented 1 year ago

@rtissera Ok after using ASan to get more details about what happens, irixxxx found a way to fix it. I quote:

Change line 1722 of pico/cd/libchdr/src/libchdr_chd.c from "goto cleanup" to "return err". heh... the file is already closed in chd_open_core_file if that fails, so it's not a memory overwrite but a double free... interesting that ASAN isn'z reporting this correctly.

It fixed it both for PicoDrive and PCSX.

irixxxx commented 1 year ago

@rtissera I can make you a PR if required.

rtissera commented 1 year ago

@irixxx sure let’s go. Thanks for the fix guys.Envoyé de mon iPhoneLe 29 nov. 2022 à 11:17, irixxxx @.***> a écrit : @rtissera I can make you a PR if required.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

irixxxx commented 1 year ago

there you are, PR #88

rtissera commented 1 year ago

Merged thanks !

Le mar. 29 nov. 2022 à 13:28, irixxxx @.***> a écrit :

there you are, PR #88 https://github.com/rtissera/libchdr/pull/88

— Reply to this email directly, view it on GitHub https://github.com/rtissera/libchdr/issues/82#issuecomment-1330555161, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAW4WBOR2PICHIJ2ZSH7RYTWKXZG3ANCNFSM575XFPVQ . You are receiving this because you were mentioned.Message ID: @.***>