[banshee] [Mtp] Fix various memory corruption issues

[Alan McGovern <alanmc src gnome org>]

commit 9812986526b3f1f984fab78d7b74c0a3e1b7aa67
Author: Alan McGovern <alan mcgovern gmail com>
Date:   Mon Sep 6 22:14:44 2010 +0100

    [Mtp] Fix various memory corruption issues
    
    In several places we made a managed copy of a libmtp struct, then freed
    the native struct, then indexed into an array on that struct. The result
    was that we indexed into freed memory because the native copy did not
    deep-copy the array which was on the native struct. This caused
    Playlists and Albums to break. Fixes #628832.

 src/Libraries/Mtp/Mtp/AbstractTrackList.cs |   49 +++++++++++++---------------
 src/Libraries/Mtp/Mtp/Album.cs             |   13 ++++++--
 src/Libraries/Mtp/Mtp/MtpDevice.cs         |    3 +-
 src/Libraries/Mtp/Mtp/Playlist.cs          |    8 +++-
 4 files changed, 41 insertions(+), 32 deletions(-)
---
diff --git a/src/Libraries/Mtp/Mtp/AbstractTrackList.cs b/src/Libraries/Mtp/Mtp/AbstractTrackList.cs
index 53a54eb..4c18c72 100644
--- a/src/Libraries/Mtp/Mtp/AbstractTrackList.cs
+++ b/src/Libraries/Mtp/Mtp/AbstractTrackList.cs
@@ -34,13 +34,11 @@ namespace Mtp
         {
             this.device = device;
             this.saved = true;
+            this.track_ids = new List<uint> ();
 
             if (tracks != IntPtr.Zero) {
-                var vals = new uint [count];
-                Marshal.Copy ((IntPtr)tracks, (int[])(object)vals, 0, (int)count);
-                track_ids = new List<uint> (vals);
-            } else {
-                track_ids = new List<uint> ();
+                for (int i = 0; i < (int) count; i++)
+                    track_ids.Add ((uint) Marshal.ReadInt32 (tracks, sizeof (int) * i));
             }
         }
 
@@ -76,27 +74,26 @@ namespace Mtp
         {
             Count = (uint) track_ids.Count;
 
-            if (TracksPtr != IntPtr.Zero) {
-                Marshal.FreeHGlobal (TracksPtr);
-                TracksPtr = IntPtr.Zero;
-            }
-
-            if (Count == 0) {
-                TracksPtr = IntPtr.Zero;
-            } else {
-                TracksPtr = Marshal.AllocHGlobal (Marshal.SizeOf (typeof (uint)) * (int)Count);
-                Marshal.Copy ((int[])(object)track_ids.ToArray (), 0, TracksPtr, (int)Count);
-            }
-
-            if (saved) {
-                saved = Update () == 0;
-            } else {
-                saved = Create () == 0;
-            }
-
-            if (TracksPtr != IntPtr.Zero) {
-                Marshal.FreeHGlobal (TracksPtr);
-                TracksPtr = IntPtr.Zero;
+            if (TracksPtr != IntPtr.Zero)
+                throw new InvalidOperationException ("TracksPtr must be NULL when Save is called");
+
+            try {
+                if (Count > 0) {
+                    TracksPtr = Marshal.AllocHGlobal (Marshal.SizeOf (typeof (uint)) * (int)Count);
+                    for (int i = 0; i < track_ids.Count; i ++)
+                        Marshal.WriteInt32 (TracksPtr, i * sizeof (int), (int) track_ids [i]);
+                }
+
+                if (saved) {
+                    saved = Update () == 0;
+                } else {
+                    saved = Create () == 0;
+                }
+            } finally {
+                if (TracksPtr != IntPtr.Zero) {
+                    Marshal.FreeHGlobal (TracksPtr);
+                    TracksPtr = IntPtr.Zero;
+                }
             }
         }
     }
diff --git a/src/Libraries/Mtp/Mtp/Album.cs b/src/Libraries/Mtp/Mtp/Album.cs
index afb8e21..090ee4d 100644
--- a/src/Libraries/Mtp/Mtp/Album.cs
+++ b/src/Libraries/Mtp/Mtp/Album.cs
@@ -41,9 +41,11 @@ namespace Mtp
 
             IntPtr ptr = LIBMTP_Get_Album_List (device.Handle);
             while (ptr != IntPtr.Zero) {
+                // Destroy the struct *after* we use it to ensure we don't access freed memory
+                // for the 'tracks' variable
                 AlbumStruct d = (AlbumStruct)Marshal.PtrToStructure(ptr, typeof(AlbumStruct));
-                LIBMTP_destroy_album_t (ptr);
                 albums.Add (new Album (device, d));
+                LIBMTP_destroy_album_t (ptr);
                 ptr = d.next;
             }
 
@@ -103,12 +105,14 @@ namespace Mtp
             Genre = genre;
             Composer = composer;
             Count = 0;
-            TracksPtr = IntPtr.Zero;
         }
 
         internal Album (MtpDevice device, AlbumStruct album) : base (device, album.tracks, album.no_tracks)
         {
+            // Once we've loaded the tracks, set the TracksPtr to NULL as it
+            // will be freed when the Album constructor is finished.
             this.album = album;
+            TracksPtr = IntPtr.Zero;
         }
 
         public override void Save ()
@@ -169,9 +173,12 @@ namespace Mtp
             if (ptr == IntPtr.Zero) {
                 return null;
             } else {
+                // Destroy the struct after we use it to prevent accessing freed memory
+                // in the 'tracks' variable
                 AlbumStruct album = (AlbumStruct) Marshal.PtrToStructure(ptr, typeof (AlbumStruct));
+                var ret = new Album (device, album);
                 LIBMTP_destroy_album_t (ptr);
-                return new Album (device, album);
+                return ret;
             }
         }
 
diff --git a/src/Libraries/Mtp/Mtp/MtpDevice.cs b/src/Libraries/Mtp/Mtp/MtpDevice.cs
index 257d17a..e594bf7 100644
--- a/src/Libraries/Mtp/Mtp/MtpDevice.cs
+++ b/src/Libraries/Mtp/Mtp/MtpDevice.cs
@@ -190,9 +190,10 @@ namespace Mtp
             List<Track> tracks = new List<Track>();
             
             while (ptr != IntPtr.Zero) {
+                // Destroy the struct after we use it to avoid potential referencing of freed memory.
                 TrackStruct track = (TrackStruct)Marshal.PtrToStructure(ptr, typeof(TrackStruct));
-                Track.DestroyTrack (ptr);
                 tracks.Add (new Track (track, this));
+                Track.DestroyTrack (ptr);
                 ptr = track.next;
             }
             
diff --git a/src/Libraries/Mtp/Mtp/Playlist.cs b/src/Libraries/Mtp/Mtp/Playlist.cs
index 5df6aba..c133c86 100644
--- a/src/Libraries/Mtp/Mtp/Playlist.cs
+++ b/src/Libraries/Mtp/Mtp/Playlist.cs
@@ -40,9 +40,11 @@ namespace Mtp
             List<Playlist> playlists = new List<Playlist> ();
             IntPtr ptr = Playlist.LIBMTP_Get_Playlist_List (device.Handle);
             while (ptr != IntPtr.Zero) {
+                // Destroy the struct *after* we use it to ensure we don't access freed memory
+                // for the 'tracks' variable
                 PlaylistStruct d = (PlaylistStruct)Marshal.PtrToStructure(ptr, typeof(PlaylistStruct));
-                LIBMTP_destroy_playlist_t (ptr);
                 playlists.Add (new Playlist (device, d));
+                LIBMTP_destroy_playlist_t (ptr);
                 ptr = d.next;
             }
             return playlists;
@@ -68,14 +70,16 @@ namespace Mtp
         public Playlist (MtpDevice device, string name) : base (device, name)
         {
             this.playlist = new PlaylistStruct ();
-            TracksPtr = IntPtr.Zero;
             Name = name;
             Count = 0;
         }
 
         internal Playlist (MtpDevice device, PlaylistStruct playlist) : base (device, playlist.tracks, playlist.no_tracks)
         {
+            // Once we've loaded the tracks, set the TracksPtr to NULL as it
+            // will be freed when the Playlist constructor is finished.
             this.playlist = playlist;
+            TracksPtr = IntPtr.Zero;
         }
 
         protected override int Create ()