Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RA 1.8.4 Segmentation fault on bad disc files #10368

Closed
pkos opened this issue Mar 29, 2020 · 17 comments · Fixed by #10376
Closed

RA 1.8.4 Segmentation fault on bad disc files #10368

pkos opened this issue Mar 29, 2020 · 17 comments · Fixed by #10376

Comments

@pkos
Copy link
Contributor

pkos commented Mar 29, 2020

https://github.com/libretro/RetroArch/blob/master/libretro-common/streams/interface_stream.c#L260

https://github.com/libretro/RetroArch/blob/master/libretro-common/streams/chd_stream.c#L313

This function will crash retroarch when a CHD bad disc is scanned that contains nothing but "\x00".

I traced it down using log entries to that function,
then i just took a downloaded RA 1.8.4 windows and tried to scan the bad file
and it crashes.

@Jamiras
Copy link
Contributor

Jamiras commented Mar 29, 2020

The file must contain more than just null characters. chdstream_open won't open a file that doesn't look like a CHD.

chd_error err = chd_open(path, CHD_OPEN_READ, NULL, &chd);
if (err != CHDERR_NONE)
goto error;

I verified this behavior by creating an "empty" 2MB chd file:

$ truncate -s 2m bad.chd

task_database_chd_get_serial bails when the file cannot be opened and it never gets into the intfstream_read code that you've referenced.

More than likely, if the CHD file is corrupt in some manner, the problem is actually in the libchdr code, not RetroArch.

Can you provide a stack trace illustrating the crash? Or more details on the file you're trying to load? No one will be able to fix the problem with the information you've provided.

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

Code:
RARCH_LOG("Got here1");
if (intfstream_read(fd, magic, MAGIC_NUMBERS[i].length_magic) > 0)
{
RARCH_LOG("Got here2");

Log:
[INFO] Written to playlist file: H:\retroarch\roms\tools\retroarch\playlists\Sega - Dreamcast.lpl
[INFO] Comparing with known magic numbers...
[INFO] Got here1

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

Or more details on the file you're trying to load? No one will be able to fix the problem with the information you've provided.

Sure:

01/02/2020 07:46 PM 5,223,256 Caesars Palace 2000 - Millennium Gold Edition v1.011 (2000)(Interplay)(NTSC)(USA)[!].chd

H:\Temp\Bad\dc>md5 "Caesars Palace 2000 - Millennium Gold Edition v1.011 (2000)(Interplay)(NTSC)(USA)[!].chd"
6238E4EB510A8EF3B2A556C9C149BAE2 Caesars Palace 2000 - Millennium Gold Edition v1.011 (2000)(Interplay)(NTSC)(USA)[!].chd

@Jamiras
Copy link
Contributor

Jamiras commented Mar 29, 2020

Those logs don't really help. That's like telling a book author that there's a spelling error in chapter 3. There's a lot of stuff that happens in that one line of code because it calls into other code.

You need to either provide the exact line of code where the crash is occurring (not a function call containing hundreds - if not thousands - of lines of code), or provide enough information for someone else to reproduce the error.

If you're smart enough to add logs to the code, why can't you provide the exact stack trace where the error occurs? All you have to do is run it in your favorite debugger and when it crashes look at the call stack.

Also, the name of the file doesn't help either. You state that the file "contains nothing but \x00.", but I've tried reproducing the problem with a file that contains nothing but null characters and couldn't, so your information is clearly incorrect (or at least incomplete). The file size may be useful, but I still need to know how you're getting passed the chdstream_open if the file "contains nothing but \x00".

A 5223256 byte file containing nothing but \x00 generates a different MD5 than the one you've listed.

$ truncate -s 5223256 bad.chd
$ ls -l bad.chd
-rw-r--r-- 1 user user 5223256 Mar 29 14:22 bad.chd
$ md5sum bad.chd
97fb6143fc01ef2d380a75b1f34fbe67 *bad.chd

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

Can you provide a stack trace illustrating the crash?

https://pastebin.com/kn95i0pi

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

A 5223256 byte file containing nothing but \x00 generates a different MD5 than the one you've listed.

$ truncate -s 5223256 bad.chd
$ ls -l bad.chd
-rw-r--r-- 1 user user 5223256 Mar 29 14:22 bad.chd
$ md5sum bad.chd
97fb6143fc01ef2d380a75b1f34fbe67 *bad.chd

The contents of the CHD when extracted contain nothing but "\x00"

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

A 5223256 byte file containing nothing but \x00 generates a different MD5 than the one you've listed.

$ truncate -s 5223256 bad.chd
$ ls -l bad.chd
-rw-r--r-- 1 user user 5223256 Mar 29 14:22 bad.chd
$ md5sum bad.chd
97fb6143fc01ef2d380a75b1f34fbe67 *bad.chd

The contents of the CHD when extracted contain nothing but "\x00"

Why did you create CHD? That's meaningless.

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

Untitled

Create a bin/cue then create the CHD.

@Jamiras
Copy link
Contributor

Jamiras commented Mar 29, 2020

Why did you create CHD? That's meaningless.

Because that's all I could do from what you provided for steps to reproduce.

This function will crash retroarch when a CHD bad disc is scanned that contains nothing but "\x00".

There's nothing in that statement that indicates the CHD was created from a file containing all 0s. It suggests the CHD itself contains nothing but 0s.

@Jamiras
Copy link
Contributor

Jamiras commented Mar 29, 2020

There's still something missing. I created a bin file that was 0x426c026c0 bytes long to match your screenshot and converted it to a CHD. The resulting file was only 432 bytes long, not 5MB. And loading it into the scanner did not cause a crash.

$ truncate -s 1186997952 bad.bin
$ cat bad.cue
FILE "bad.bin" BINARY
  TRACK 01 MODE1/2048
      INDEX 01 00:00:00
$ ..\Tools\chdman.exe createcd -i bad.cue -o bad.chd
chdman - MAME Compressed Hunks of Data (CHD) manager 0.212 (mame0212)
Output CHD:   bad.chd
Input file:   bad.cue
Input tracks: 1
Input length: 128:47:63
Compression:  cdlz (CD LZMA), cdzl (CD Deflate), cdfl (CD FLAC)
Logical size: 1,418,831,424
Compression complete ... final ratio = 0.0%
$ ls -l bad.chd
4 -rw-r--r-- 1 user user 432 Mar 29 15:25 bad.chd

@Jamiras
Copy link
Contributor

Jamiras commented Mar 29, 2020

The debugger information that you've provided shows the issue is indeed in libchdr.

Thread 7 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 7204.0x5114]
0x0000000000a05d41 in hunk_read_into_memory ()

https://github.com/rtissera/libchdr/blob/ad32a2deeb4a8b336d6e73d2bc79109005873941/src/chd.c#L2006
It's still unclear exactly which line is causing the crash.

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

Thanks, what else do you think I could provide that might help?

@pkos
Copy link
Contributor Author

pkos commented Mar 29, 2020

Just so you know, these CHD files are actual conversions from bin/cue... I've found more than one example of these '\x00' files so they can actually be created by other people.

@i30817
Copy link
Contributor

i30817 commented Mar 31, 2020

give the damn file. If it's made from a corrupt all 0 files it can't be 'copyrighted'. Data input bugs need the wrong data to test.

And libchd should be fuzzed. As well as many other things in RA.

@pkos
Copy link
Contributor Author

pkos commented Mar 31, 2020

dc.zip

Just to be clear these are all '\x00' files... not anything copyrighted.

@Jamiras
Copy link
Contributor

Jamiras commented Mar 31, 2020

Thanks. So much easier to debug when you have the proper information.

The problem is the version of libchdr that we're using is ignoring the error code returned by decompress_v5_map when reading the header:

err = decompress_v5_map(newchd, &(newchd->header));
(void)err;

Which leads to a null dereference later when trying to read the file.

This was fixed upstream nearly two years ago: rtissera/libchdr@e1acac6#diff-b4791a43102472fe8d3959f86bd376c4R1317

I'm going to create a PR to address this specific change, but it might be time to upgrade the entire library.

@pkos
Copy link
Contributor Author

pkos commented Mar 31, 2020

This is a PR for new scanner code... if you're poking around with it mind taking a look or perhaps a test clone? Coincidentally, while testing the scanner code, I ran across these files that kept crashing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants