Limit size of loaded file systems #2610

JosJuice · 2015-06-14T08:37:21Z

If a disc image is corrupt in a specific way, Dolphin will try to allocate a lot of memory, making it crash. (2d5d5fa#commitcomment-11147057)

To avoid that, this change adds an artificial limit for the size of file systems that Dolphin will try to load.

Source/Core/DiscIO/FileSystemGCWii.cpp

@@ -270,6 +271,18 @@ void CFileSystemGCWii::InitFileSystem()
 	if (!Root.IsDirectory())
 		return;

+	if (Root.m_FileSize > 2 * 1024 * 1024)


Source/Core/DiscIO/FileSystemGCWii.cpp

+		// Without this check, Dolphin may crash by trying to allocate too much memory
+		// when loading the filesystems of certain corrupt disc images. 12 bytes
+		// (the size of a file entry) times 2 * 1024 * 1024 is equal to the size of
+		// main GC/Wii RAM (24 MiB), and no file system should use anywhere near


Source/Core/DiscIO/FileSystemGCWii.cpp

@@ -270,6 +271,19 @@ void CFileSystemGCWii::InitFileSystem()
 	if (!Root.IsDirectory())
 		return;

+	static const u32 ARBITRARY_FILE_SYSTEM_SIZE_LIMIT = 2 * 1024 * 1024;
+	if (Root.m_FileSize > ARBITRARY_FILE_SYSTEM_SIZE_LIMIT)


shuffle2 · 2015-10-04T01:16:51Z

Allocating >= 4GB of ram is no problem, that is what virtual memory is for.
Why not just see if the allocation failed and then fail?
ARBITRARY_FILE_SYSTEM_SIZE_LIMIT seems pretty small.

JosJuice · 2015-10-04T08:14:17Z

After allocating that much memory, Dolphin will try to write to it all. If the user doesn't have enough RAM, that's at least going to be very slow.

How do I check if the allocation fails? I thought exception handling was disabled in Dolphin...

I've increased the limit from 24 MiB to 120 MiB.

If a disc image is malformed in a specific way, Dolphin will try to allocate a lot of memory, making it crash. To avoid that, this change adds an artificial limit for the size of file systems that Dolphin will try to load.

shuffle2 · 2015-10-04T08:43:05Z

I still dont think putting a limit on file size (below disc size) is a good idea. there is undoubtedly some stupid game out there that has just one giant compressed file or something.
Other approaches:

verify disc hash structure (present on wii discs)
sanity-check FST values (e.g. go through the entries and see if total size is less than disc size, no entries overlap, etc)

shuffle2 · 2015-10-04T08:46:39Z

Actually, looking closer, this is just "filesize" for Root...which is the count of entries, nothing to do with filesize.
The FST size should be given elsewhere on the disc. if root.size * entry_size != fst_size then you know it's broken, no?

JosJuice · 2015-10-04T09:04:35Z

Verifying disc hashes will catch unintentionally malformed disc images but not intentionally malformed ones. Is that good enough, and will such a change be accepted in stable? Will such a change even be made in time for stable? (EDIT: Also won't catch any problems with GC discs.)

The allocation happens before I can read the FST entries. I suppose it's possible to skip the reserve and just allocate memory gradually, but I'd like to have a solution that doesn't break with PR #2820.

No, because fst_size == root.size * entry_size + name_table_size. Besides, I can't rely on fst_size being correct.

In short, your suggestions make sense (except for root.size * entry_size != fst_size specifically) and would catch most problems. However, just having a limit will catch all allocation problems, and it will not cause any problems with valid games. Games need to load FSTs into RAM to run, and 120 MiB is larger than GC/Wii RAM, so any file systems that this PR won't load won't work on a real console and can't run in Dolphin.

Helios747 · 2015-10-18T20:16:52Z

Ping @Armada651 to get this merged into stable. This will clear a blocker.

CrossVR · 2015-10-19T08:42:37Z

@aserna3 Sure, but first get this merged into master.

JosJuice · 2015-10-19T11:16:36Z

@Armada651 Do you mean that we should get this PR merged or that I should make a new PR for master? This PR is for stable.

CrossVR · 2015-10-19T13:23:49Z

Oh I didn't see this was for stable, so should this be picked for master too?

JosJuice · 2015-10-19T13:33:19Z

Sure, if you're fine with doing cherry-picks to master. You said you weren't going to do any when we were talking about #2815, but if that's changed, then go ahead. (After the PR is merged, that is)

CrossVR · 2015-10-19T14:05:49Z

@JosJuice I'd still prefer not to.

JosJuice · 2015-10-28T20:51:29Z

Making PRs for stable gets too messy with our release process. I've remade this PR for master: #3206

lioncash reviewed Jun 17, 2015
View reviewed changes

Source/Core/DiscIO/FileSystemGCWii.cpp

@@ -270,6 +271,18 @@ void CFileSystemGCWii::InitFileSystem()

if (!Root.IsDirectory())

return;

if (Root.m_FileSize > 2 * 1024 * 1024)

This comment was marked as off-topic.

Sign in to view

JosJuice force-pushed the limit-filesystem-size branch from 0697356 to bf664c9 Compare June 17, 2015 19:16

bb010g reviewed Jun 24, 2015
View reviewed changes

JosJuice force-pushed the limit-filesystem-size branch from bf664c9 to 18a94c1 Compare July 12, 2015 07:06

degasus reviewed Jul 23, 2015
View reviewed changes

JosJuice mentioned this pull request Aug 9, 2015

Filesystem redesign and performance improvements #2820

Merged

Rukario mentioned this pull request Aug 15, 2015

DolphinWX: Fix memory leaks occurring in ISOProperties #2858

Merged

JosJuice force-pushed the limit-filesystem-size branch from 18a94c1 to 6b2cb1e Compare October 4, 2015 08:14

Limit size of loaded file systems

aaefbdc

If a disc image is malformed in a specific way, Dolphin will try to allocate a lot of memory, making it crash. To avoid that, this change adds an artificial limit for the size of file systems that Dolphin will try to load.

JosJuice force-pushed the limit-filesystem-size branch from 6b2cb1e to aaefbdc Compare October 4, 2015 08:18

JosJuice mentioned this pull request Oct 28, 2015

Limit size of loaded file systems #3206

Merged

JosJuice closed this Oct 28, 2015

JosJuice deleted the limit-filesystem-size branch November 18, 2015 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit size of loaded file systems #2610

Limit size of loaded file systems #2610

JosJuice commented Jun 14, 2015

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

shuffle2 commented Oct 4, 2015

JosJuice commented Oct 4, 2015

shuffle2 commented Oct 4, 2015

shuffle2 commented Oct 4, 2015

JosJuice commented Oct 4, 2015

Helios747 commented Oct 18, 2015

CrossVR commented Oct 19, 2015

JosJuice commented Oct 19, 2015

CrossVR commented Oct 19, 2015

JosJuice commented Oct 19, 2015

CrossVR commented Oct 19, 2015

JosJuice commented Oct 28, 2015

Limit size of loaded file systems #2610

Limit size of loaded file systems #2610

Conversation

JosJuice commented Jun 14, 2015

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

shuffle2 commented Oct 4, 2015

JosJuice commented Oct 4, 2015

shuffle2 commented Oct 4, 2015

shuffle2 commented Oct 4, 2015

JosJuice commented Oct 4, 2015

Helios747 commented Oct 18, 2015

CrossVR commented Oct 19, 2015

JosJuice commented Oct 19, 2015

CrossVR commented Oct 19, 2015

JosJuice commented Oct 19, 2015

CrossVR commented Oct 19, 2015

JosJuice commented Oct 28, 2015