Converted more new/delete pairs to unique_ptr. This actually made me notice

a memory leak where the Console was never being deleted.

For FSNode read, change 'uInt8[]' arrays to BytePtr, which is an alias to
a unique_ptr array.  Again, this enables automatic deletion when the object
goes out of scope.


git-svn-id: svn://svn.code.sf.net/p/stella/code/trunk@3173 8b62c5a3-ac7e-4cc8-8f21-d9a121418aba

Changes.txt

@@ -14,6 +14,9 @@
 
 4.6.1 to 4.7: (mmm dd, 2015)
 
+  * Fixed memory leak; the game console wasn't being closed after exiting
+    a ROM.
+
   * Updated included PNG library to latest stable version.
 
 -Have fun!

src/common/FSNodeZIP.cxx

@@ -175,7 +175,7 @@ bool FilesystemNodeZIP::getChildren(AbstractFSList& myList, ListMode mode,
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-uInt32 FilesystemNodeZIP::read(uInt8*& image) const
+uInt32 FilesystemNodeZIP::read(BytePtr& image) const
 {
   switch(_error)
   {

src/common/FSNodeZIP.hxx

@@ -66,7 +66,7 @@ class FilesystemNodeZIP : public AbstractFSNode
     bool getChildren(AbstractFSList& list, ListMode mode, bool hidden) const;
     AbstractFSNode* getParent() const;
 
-    uInt32 read(uInt8*& image) const;
+    uInt32 read(BytePtr& image) const;
 
   private:
     FilesystemNodeZIP(const string& zipfile, const string& virtualpath,

src/common/ZipHandler.cxx

@@ -87,7 +87,7 @@ string ZipHandler::next()
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-uInt32 ZipHandler::decompress(uInt8*& image)
+uInt32 ZipHandler::decompress(BytePtr& image)
 {
   static const char* zip_error_s[] = {
     "ZIPERR_NONE",
@@ -104,17 +104,13 @@ uInt32 ZipHandler::decompress(uInt8*& image)
   if(myZip)
   {
     uInt32 length = myZip->header.uncompressed_length;
-    image = new uInt8[length];
+    image = make_ptr<uInt8[]>(length);
 
-    ZipHandler::zip_error err = zip_file_decompress(myZip, image, length);
+    ZipHandler::zip_error err = zip_file_decompress(myZip, image.get(), length);
     if(err == ZIPERR_NONE)
       return length;
     else
-    {
-      delete[] image;  image = nullptr;
-
       throw runtime_error(zip_error_s[err]);
-    }
   }
   else
     throw runtime_error("Invalid ZIP archive");

src/common/ZipHandler.hxx

@@ -81,7 +81,7 @@ class ZipHandler
 
     // Decompress the currently selected file and return its length
     // An exception will be thrown on any errors
-    uInt32 decompress(uInt8*& image);
+    uInt32 decompress(BytePtr& image);
 
     // Answer the number of ROM files found in the archive
     // Currently, this means files with extension a26/bin/rom

src/common/bspf.hxx

@@ -63,6 +63,7 @@ using IntArray = vector<Int32>;
 using BoolArray = vector<bool>;
 using ByteArray = vector<uInt8>;
 using StringList = vector<string>;
+using BytePtr = unique_ptr<uInt8[]>;
 
 // Defines to help with path handling
 #if (defined(BSPF_UNIX) || defined(BSPF_MAC_OSX))

src/emucore/Cart.cxx

@@ -70,12 +70,17 @@
 #endif
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-Cartridge* Cartridge::create(const uInt8* image, uInt32 size, string& md5,
+unique_ptr<Cartridge> Cartridge::create(const BytePtr& img, uInt32 size,
-     string& dtype, string& id, const OSystem& osystem, Settings& settings)
+    string& md5, string& dtype, string& id,
+    const OSystem& osystem, Settings& settings)
 {
-  Cartridge* cartridge = nullptr;
+  unique_ptr<Cartridge> cartridge;
   string type = dtype;
 
+  // For now, we convert from BytePtr to the raw pointer
+  // Eventually, the Cartridge hierarchy should probably use BytePtr directly
+  const uInt8* image = img.get();
+
   // Collect some info about the ROM
   ostringstream buf;
 
@@ -181,81 +186,81 @@ Cartridge* Cartridge::create(const uInt8* image, uInt32 size, string& md5,
 
   // We should know the cart's type by now so let's create it
   if(type == "0840")
-    cartridge = new Cartridge0840(image, size, settings);
+    cartridge = make_ptr<Cartridge0840>(image, size, settings);
   else if(type == "2K")
-    cartridge = new Cartridge2K(image, size, settings);
+    cartridge = make_ptr<Cartridge2K>(image, size, settings);
   else if(type == "3E")
-    cartridge = new Cartridge3E(image, size, settings);
+    cartridge = make_ptr<Cartridge3E>(image, size, settings);
   else if(type == "3F")
-    cartridge = new Cartridge3F(image, size, settings);
+    cartridge = make_ptr<Cartridge3F>(image, size, settings);
   else if(type == "4A50")
-    cartridge = new Cartridge4A50(image, size, settings);
+    cartridge = make_ptr<Cartridge4A50>(image, size, settings);
   else if(type == "4K")
-    cartridge = new Cartridge4K(image, size, settings);
+    cartridge = make_ptr<Cartridge4K>(image, size, settings);
   else if(type == "4KSC")
-    cartridge = new Cartridge4KSC(image, size, settings);
+    cartridge = make_ptr<Cartridge4KSC>(image, size, settings);
   else if(type == "AR")
-    cartridge = new CartridgeAR(image, size, settings);
+    cartridge = make_ptr<CartridgeAR>(image, size, settings);
   else if(type == "CM")
-    cartridge = new CartridgeCM(image, size, settings);
+    cartridge = make_ptr<CartridgeCM>(image, size, settings);
   else if(type == "CTY")
-    cartridge = new CartridgeCTY(image, size, osystem);
+    cartridge = make_ptr<CartridgeCTY>(image, size, osystem);
   else if(type == "CV")
-    cartridge = new CartridgeCV(image, size, settings);
+    cartridge = make_ptr<CartridgeCV>(image, size, settings);
   else if(type == "DASH")
-    cartridge = new CartridgeDASH(image, size, settings);
+    cartridge = make_ptr<CartridgeDASH>(image, size, settings);
   else if(type == "DPC")
-    cartridge = new CartridgeDPC(image, size, settings);
+    cartridge = make_ptr<CartridgeDPC>(image, size, settings);
   else if(type == "DPC+")
-    cartridge = new CartridgeDPCPlus(image, size, settings);
+    cartridge = make_ptr<CartridgeDPCPlus>(image, size, settings);
   else if(type == "E0")
-    cartridge = new CartridgeE0(image, size, settings);
+    cartridge = make_ptr<CartridgeE0>(image, size, settings);
   else if(type == "E7")
-    cartridge = new CartridgeE7(image, size, settings);
+    cartridge = make_ptr<CartridgeE7>(image, size, settings);
   else if(type == "EF")
-    cartridge = new CartridgeEF(image, size, settings);
+    cartridge = make_ptr<CartridgeEF>(image, size, settings);
   else if(type == "EFSC")
-    cartridge = new CartridgeEFSC(image, size, settings);
+    cartridge = make_ptr<CartridgeEFSC>(image, size, settings);
   else if(type == "BF")
-    cartridge = new CartridgeBF(image, size, settings);
+    cartridge = make_ptr<CartridgeBF>(image, size, settings);
   else if(type == "BFSC")
-    cartridge = new CartridgeBFSC(image, size, settings);
+    cartridge = make_ptr<CartridgeBFSC>(image, size, settings);
   else if(type == "DF")
-    cartridge = new CartridgeDF(image, size, settings);
+    cartridge = make_ptr<CartridgeDF>(image, size, settings);
   else if(type == "DFSC")
-    cartridge = new CartridgeDFSC(image, size, settings);
+    cartridge = make_ptr<CartridgeDFSC>(image, size, settings);
   else if(type == "F0" || type == "MB")
-    cartridge = new CartridgeF0(image, size, settings);
+    cartridge = make_ptr<CartridgeF0>(image, size, settings);
   else if(type == "F4")
-    cartridge = new CartridgeF4(image, size, settings);
+    cartridge = make_ptr<CartridgeF4>(image, size, settings);
   else if(type == "F4SC")
-    cartridge = new CartridgeF4SC(image, size, settings);
+    cartridge = make_ptr<CartridgeF4SC>(image, size, settings);
   else if(type == "F6")
-    cartridge = new CartridgeF6(image, size, settings);
+    cartridge = make_ptr<CartridgeF6>(image, size, settings);
   else if(type == "F6SC")
-    cartridge = new CartridgeF6SC(image, size, settings);
+    cartridge = make_ptr<CartridgeF6SC>(image, size, settings);
   else if(type == "F8")
-    cartridge = new CartridgeF8(image, size, md5, settings);
+    cartridge = make_ptr<CartridgeF8>(image, size, md5, settings);
   else if(type == "F8SC")
-    cartridge = new CartridgeF8SC(image, size, settings);
+    cartridge = make_ptr<CartridgeF8SC>(image, size, settings);
   else if(type == "FA" || type == "FASC")
-    cartridge = new CartridgeFA(image, size, settings);
+    cartridge = make_ptr<CartridgeFA>(image, size, settings);
   else if(type == "FA2")
-    cartridge = new CartridgeFA2(image, size, osystem);
+    cartridge = make_ptr<CartridgeFA2>(image, size, osystem);
   else if(type == "FE")
-    cartridge = new CartridgeFE(image, size, settings);
+    cartridge = make_ptr<CartridgeFE>(image, size, settings);
   else if(type == "MC")
-    cartridge = new CartridgeMC(image, size, settings);
+    cartridge = make_ptr<CartridgeMC>(image, size, settings);
   else if(type == "MDM")
-    cartridge = new CartridgeMDM(image, size, settings);
+    cartridge = make_ptr<CartridgeMDM>(image, size, settings);
   else if(type == "UA")
-    cartridge = new CartridgeUA(image, size, settings);
+    cartridge = make_ptr<CartridgeUA>(image, size, settings);
   else if(type == "SB")
-    cartridge = new CartridgeSB(image, size, settings);
+    cartridge = make_ptr<CartridgeSB>(image, size, settings);
   else if(type == "WD")
-    cartridge = new CartridgeWD(image, size, settings);
+    cartridge = make_ptr<CartridgeWD>(image, size, settings);
   else if(type == "X07")
-    cartridge = new CartridgeX07(image, size, settings);
+    cartridge = make_ptr<CartridgeX07>(image, size, settings);
   else if(dtype == "WRONG_SIZE")
     throw runtime_error("Invalid cart size for type '" + type + "'");
   else

src/emucore/Cart.hxx

@@ -60,9 +60,10 @@ class Cartridge : public Device
       @param settings The settings associated with the system
       @return   Pointer to the new cartridge object allocated on the heap
     */
-    static Cartridge* create(const uInt8* image, uInt32 size, string& md5,
+    static unique_ptr<Cartridge>
-                             string& dtype, string& id,
+        create(const BytePtr& image, uInt32 size,
-                             const OSystem& system, Settings& settings);
+               string& md5, string& dtype, string& id,
+               const OSystem& system, Settings& settings);
 
     /**
       Create a new cartridge
@@ -257,6 +258,7 @@ class Cartridge : public Device
 
       @param image  A pointer to the ROM image
       @param size   The size of the ROM image 
+
       @return The "best guess" for the cartridge type
     */
     static string autodetectType(const uInt8* image, uInt32 size);

src/emucore/Console.cxx

@@ -66,10 +66,12 @@
 #include "Console.hxx"
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-Console::Console(OSystem& osystem, Cartridge* cart, const Properties& props)
+Console::Console(OSystem& osystem, unique_ptr<Cartridge>& cart,
+                 const Properties& props)
   : myOSystem(osystem),
     myEvent(osystem.eventHandler().event()),
     myProperties(props),
+    myCart(std::move(cart)),
     myDisplayFormat(""),  // Unknown TV format @ start
     myFramerate(0.0),     // Unknown framerate @ start
     myCurrentFormat(0),   // Unknown format @ start
@@ -82,7 +84,6 @@ Console::Console(OSystem& osystem, Cartridge* cart, const Properties& props)
   my6502 = make_ptr<M6502>(myOSystem.settings());
   myRiot = make_ptr<M6532>(*this, myOSystem.settings());
   myTIA  = make_ptr<TIA>(*this, myOSystem.sound(), myOSystem.settings());
-  myCart = unique_ptr<Cartridge>(cart);
   mySwitches = make_ptr<Switches>(myEvent, myProperties);
 
   // Construct the system and components

src/emucore/Console.hxx

@@ -69,7 +69,8 @@ class Console : public Serializable
       @param cart     The cartridge to use with this console
       @param props    The properties for the cartridge  
     */
-    Console(OSystem& osystem, Cartridge* cart, const Properties& props);
+    Console(OSystem& osystem, unique_ptr<Cartridge>& cart,
+            const Properties& props);
  
     /**
       Destructor

src/emucore/FSNode.cxx

@@ -175,7 +175,7 @@ bool FilesystemNode::rename(const string& newfile)
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-uInt32 FilesystemNode::read(uInt8*& image) const
+uInt32 FilesystemNode::read(BytePtr& image) const
 {
   uInt32 size = 0;
 
@@ -191,15 +191,13 @@ uInt32 FilesystemNode::read(uInt8*& image) const
   gzFile f = gzopen(getPath().c_str(), "rb");
   if(f)
   {
-    image = new uInt8[512 * 1024];
+    image = make_ptr<uInt8[]>(512 * 1024);
-    size = gzread(f, image, 512 * 1024);
+    size = gzread(f, image.get(), 512 * 1024);
     gzclose(f);
 
     if(size == 0)
-    {
-      delete[] image;  image = nullptr;
       throw runtime_error("Zero-byte file");
-    }
+
     return size;
   }
   else

src/emucore/FSNode.hxx

@@ -229,14 +229,13 @@ class FilesystemNode
     /**
      * Read data (binary format) into the given buffer.
      *
-     * @param buffer  The buffer to containing the data
+     * @param buffer  The buffer to contain the data.
-     *                This will be allocated by the method, and must be
+     *
-     *                freed by the caller.
      * @return  The number of bytes read (0 in the case of failure)
      *          This method can throw exceptions, and should be used inside
      *          a try-catch block.
      */
-    virtual uInt32 read(uInt8*& buffer) const;
+    virtual uInt32 read(BytePtr& buffer) const;
 
     /**
      * The following methods are almost exactly the same as the various
@@ -380,7 +379,7 @@ class AbstractFSNode
      *          This method can throw exceptions, and should be used inside
      *          a try-catch block.
      */
-    virtual uInt32 read(uInt8*& buffer) const { return 0; }
+    virtual uInt32 read(BytePtr& buffer) const { return 0; }
 
     /**
      * The parent node of this directory.

src/emucore/MD5.cxx

@@ -325,6 +325,12 @@ static void MD5_memset(POINTER output, int value, uInt32 len)
  ((char *)output)[i] = (char)value;
 }
 
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+string hash(const BytePtr& buffer, uInt32 length)
+{
+  return hash(buffer.get(), length);
+}
+
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 string hash(const uInt8* buffer, uInt32 length)
 {
@@ -349,7 +355,7 @@ string hash(const uInt8* buffer, uInt32 length)
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 string hash(const FilesystemNode& node)
 {
-  uInt8* image = nullptr;
+  BytePtr image;
   uInt32 size = 0;
   try
   {
@@ -361,7 +367,6 @@ string hash(const FilesystemNode& node)
   }
 
   const string& md5 = hash(image, size);
-  delete[] image;
   return md5;
 }
 

src/emucore/MD5.hxx

@@ -33,6 +33,7 @@ namespace MD5 {
   @param length The length of the message
   @return The message-digest
 */
+string hash(const BytePtr& buffer, uInt32 length);
 string hash(const uInt8* buffer, uInt32 length);
 
 /**

src/emucore/OSystem.cxx

@@ -473,9 +473,9 @@ unique_ptr<Console> OSystem::openConsole(const FilesystemNode& romfile,
   unique_ptr<Console> console;
 
   // Open the cartridge image and read it in
-  uInt8* image = nullptr;
+  BytePtr image;
   uInt32 size  = 0;
-  if((image = openROM(romfile, md5, size)) != 0)
+  if((image = openROM(romfile, md5, size)) != nullptr)
   {
     // Get a valid set of properties, including any entered on the commandline
     // For initial creation of the Cart, we're only concerned with the BS type
@@ -494,7 +494,7 @@ unique_ptr<Console> OSystem::openConsole(const FilesystemNode& romfile,
     // Now create the cartridge
     string cartmd5 = md5;
     type = props.get(Cartridge_Type);
-    Cartridge* cart =
+    unique_ptr<Cartridge> cart =
       Cartridge::create(image, size, cartmd5, type, id, *this, *mySettings);
 
     // It's possible that the cart created was from a piece of the image,
@@ -532,10 +532,6 @@ unique_ptr<Console> OSystem::openConsole(const FilesystemNode& romfile,
       console = make_ptr<Console>(*this, cart, props);
   }
 
-  // Free the image since we don't need it any longer
-  if(image != 0 && size > 0)
-    delete[] image;
-
   return console;
 }
 
@@ -548,24 +544,20 @@ void OSystem::closeConsole()
     // If a previous console existed, save cheats before creating a new one
     myCheatManager->saveCheats(myConsole->properties().get(Cartridge_MD5));
   #endif
-    myConsole.release();
   }
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-uInt8* OSystem::openROM(const FilesystemNode& rom, string& md5, uInt32& size)
+BytePtr OSystem::openROM(const FilesystemNode& rom, string& md5, uInt32& size)
 {
   // This method has a documented side-effect:
   // It not only loads a ROM and creates an array with its contents,
   // but also adds a properties entry if the one for the ROM doesn't
   // contain a valid name
 
-  uInt8* image = nullptr;
+  BytePtr image;
   if((size = rom.read(image)) == 0)
-  {
-    delete[] image;
     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

src/emucore/OSystem.hxx

@@ -20,7 +20,6 @@
 #ifndef OSYSTEM_HXX
 #define OSYSTEM_HXX
 
-class Cartridge;
 class CheatManager;
 class CommandMenu;
 class Console;
@@ -36,6 +35,7 @@ class Sound;
 class StateManager;
 class VideoDialog;
 
+#include "Cart.hxx"
 #include "FSNode.hxx"
 #include "FrameBuffer.hxx"
 #include "PNGLibrary.hxx"
@@ -558,10 +558,9 @@ class OSystem
                     (will be recalculated if necessary)
       @param size   The amount of data read into the image array
 
-      @return  Pointer to the array, with size >=0 indicating valid data
+      @return  Unique pointer to the array
-               (calling method is responsible for deleting it)
     */
-    uInt8* openROM(const FilesystemNode& rom, string& md5, uInt32& size);
+    BytePtr openROM(const FilesystemNode& rom, string& md5, uInt32& size);
 
     /**
       Gets all possible info about the given console.