From fed3fa22412222a2d2606b3fd9cbafb0c356c7c5 Mon Sep 17 00:00:00 2001 From: Stephen Anthony Date: Sat, 16 Apr 2022 19:25:50 -0230 Subject: [PATCH] Fix parsing large MVC files causing lockup/crash in GUI (fixes # 829). Still TODO is deal with large files other than MVC, and also ZIP files. At least the logic for analyzing size is now in one method (OSystem::openROM). --- src/emucore/FSNode.cxx | 4 ++-- src/emucore/MD5.cxx | 20 +--------------- src/emucore/MD5.hxx | 21 ++++++----------- src/emucore/OSystem.cxx | 48 +++++++++++++++++++++++++++++--------- src/emucore/OSystem.hxx | 23 ++++++++++++++++-- src/gui/LauncherDialog.cxx | 2 +- src/gui/RomAuditDialog.cxx | 2 +- 7 files changed, 70 insertions(+), 50 deletions(-) diff --git a/src/emucore/FSNode.cxx b/src/emucore/FSNode.cxx index 696e2c0f1..03e536aca 100644 --- a/src/emucore/FSNode.cxx +++ b/src/emucore/FSNode.cxx @@ -351,8 +351,8 @@ size_t FilesystemNode::read(ByteBuffer& buffer, size_t size) const if (sizeRead == 0) throw runtime_error("Zero-byte file"); - else if (size != 0) - sizeRead = size; + else if (size > 0) // If a requested size to read is provided, honour it + sizeRead = std::min(sizeRead, size); buffer = make_unique(sizeRead); in.read(reinterpret_cast(buffer.get()), sizeRead); diff --git a/src/emucore/MD5.cxx b/src/emucore/MD5.cxx index c6c18b72e..92df3e013 100644 --- a/src/emucore/MD5.cxx +++ b/src/emucore/MD5.cxx @@ -317,7 +317,7 @@ string hash(const ByteBuffer& buffer, size_t length) // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - string hash(const uInt8* buffer, size_t length) { - constexpr char hex[] = "0123456789abcdef"; + static constexpr char hex[] = "0123456789abcdef"; MD5_CTX context{}; uInt8 md5[16] = {0}; const uInt32 len32 = static_cast(length); // Always use 32-bit for now @@ -336,22 +336,4 @@ string hash(const uInt8* buffer, size_t length) return result; } -// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -string hash(const FilesystemNode& node) -{ - ByteBuffer image; - size_t size = 0; - try - { - size = node.read(image); - } - catch(...) - { - return EmptyString; - } - - const string& md5 = hash(image, size); - return md5; -} - } // Namespace MD5 diff --git a/src/emucore/MD5.hxx b/src/emucore/MD5.hxx index f367b4ba7..55fe71145 100644 --- a/src/emucore/MD5.hxx +++ b/src/emucore/MD5.hxx @@ -33,29 +33,22 @@ namespace MD5 { Based on the size of data we currently use, this may never actually happen. - @param buffer The message to compute the digest of - @param length The length of the message - @return The message-digest + @param buffer The message to compute the digest of + @param length The length of the message + + @return The message-digest */ string hash(const ByteBuffer& buffer, size_t length); string hash(const uInt8* buffer, size_t length); /** Dito. - @param buffer The message to compute the digest of - @return The message - digest + @param buffer The message to compute the digest of + + @return The message - digest */ string hash(const string& buffer); - /** - Get the MD5 Message-Digest of the file contained in 'node'. - The digest consists of 32 hexadecimal digits. - - @param node The file node to compute the digest of - @return The message-digest - */ - string hash(const FilesystemNode& node); - } // Namespace MD5 #endif diff --git a/src/emucore/OSystem.cxx b/src/emucore/OSystem.cxx index 0a8713739..f3b10fd09 100644 --- a/src/emucore/OSystem.cxx +++ b/src/emucore/OSystem.cxx @@ -753,6 +753,34 @@ ByteBuffer OSystem::openROM(const FilesystemNode& rom, string& md5, size_t& size // but also adds a properties entry if the one for the ROM doesn't // contain a valid name + ByteBuffer image = openROM(rom, size); + if(image) + { + // If we get to this point, we know we have a valid file to open + // Now we make sure that the file has a valid properties entry + // To save time, only generate an MD5 if we really need one + if(md5 == "") + md5 = MD5::hash(image, size); + + // Make sure to load a per-ROM properties entry, if one exists + myPropSet->loadPerROM(rom, md5); + } + + return image; +} + +// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +string OSystem::getROMMD5(const FilesystemNode& rom) const +{ + size_t size = 0; + const ByteBuffer image = openROM(rom, size); + + return image ? MD5::hash(image, size) : EmptyString; +} + +// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +ByteBuffer OSystem::openROM(const FilesystemNode& rom, size_t& size) const +{ // First check if this is a 'streaming' ROM (one where we only read // a portion of the file) const size_t sizeToRead = CartDetector::isProbablyMVC(rom); @@ -762,17 +790,15 @@ ByteBuffer OSystem::openROM(const FilesystemNode& rom, string& md5, size_t& size // if it's not a ZIP file (that size should be higher; still TBD) ByteBuffer image; - if((size = rom.read(image, sizeToRead)) == 0) - return nullptr; - - // If we get to this point, we know we have a valid file to open - // Now we make sure that the file has a valid properties entry - // To save time, only generate an MD5 if we really need one - if(md5 == "") - md5 = MD5::hash(image, size); - - // Make sure to load a per-ROM properties entry, if one exists - myPropSet->loadPerROM(rom, md5); + try + { + if((size = rom.read(image, sizeToRead)) == 0) + return nullptr; + } + catch(const runtime_error& e) + { + cerr << "ERROR: Couldn't open ROM (" << e.what() << ")"; + } return image; } diff --git a/src/emucore/OSystem.hxx b/src/emucore/OSystem.hxx index 8c2698f8f..5a548bd91 100644 --- a/src/emucore/OSystem.hxx +++ b/src/emucore/OSystem.hxx @@ -338,6 +338,15 @@ class OSystem */ ByteBuffer openROM(const FilesystemNode& rom, string& md5, size_t& size); + /** + Open the given ROM and return the MD5sum of the data. + + @param rom The file node of the ROM to open (contains path) + + @return MD5 of the ROM image (if valid), otherwise EmptyString + */ + string getROMMD5(const FilesystemNode& rom) const; + /** Creates a new game console from the specified romfile, and correctly initializes the system state to start emulation of the Console. @@ -596,7 +605,6 @@ class OSystem static bool ourOverrideBaseDirWithApp; private: - /** This method should be called to initiate the process of loading settings from the config file. It takes care of loading settings, applying @@ -609,13 +617,24 @@ class OSystem */ void createSound(); + /** + Open the given ROM and return an array containing its contents. + This method takes care of using only a valid size for the + + @param romfile The file node of the ROM to open (contains path) + @param size The amount of data read into the image array + + @return Unique pointer to the array, otherwise nullptr + */ + ByteBuffer openROM(const FilesystemNode& romfile, size_t& size) const; + /** Creates an actual Console object based on the given info. @param romfile The file node of the ROM to use (contains path) @param md5 The MD5sum of the ROM - @return The actual Console object, otherwise nullptr. + @return The actual Console object, otherwise nullptr */ unique_ptr openConsole(const FilesystemNode& romfile, string& md5); diff --git a/src/gui/LauncherDialog.cxx b/src/gui/LauncherDialog.cxx index 208f94b79..125185196 100644 --- a/src/gui/LauncherDialog.cxx +++ b/src/gui/LauncherDialog.cxx @@ -395,7 +395,7 @@ const string& LauncherDialog::selectedRomMD5() // Lookup MD5, and if not present, cache it const auto iter = myMD5List.find(currentNode().getPath()); if(iter == myMD5List.end()) - myMD5List[currentNode().getPath()] = MD5::hash(currentNode()); + myMD5List[currentNode().getPath()] = instance().getROMMD5(currentNode()); return myMD5List[currentNode().getPath()]; } diff --git a/src/gui/RomAuditDialog.cxx b/src/gui/RomAuditDialog.cxx index ec1aed9df..50ca6f846 100644 --- a/src/gui/RomAuditDialog.cxx +++ b/src/gui/RomAuditDialog.cxx @@ -139,7 +139,7 @@ void RomAuditDialog::auditRoms() // Calculate the MD5 so we can get the rest of the info // from the PropertiesSet (stella.pro) - const string& md5 = MD5::hash(files[idx]); + const string& md5 = instance().getROMMD5(files[idx]); if(instance().propSet().getMD5(md5, props)) { const string& name = props.get(PropType::Cart_Name);