Leak fixes

Leak fixes

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

Hello I discovered some memory leaks. Here is the fix
The main one was is that if a hash collision occurs new cache entry 
overwrites the old one but the old one wasn't freed. When loading a big 
file it was leaking huge amounts of memory

-- 

Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: leakfix.diff --]
[-- Type: text/x-patch, Size: 2466 bytes --]

Index: kern/disk.c
===================================================================
--- kern/disk.c	(revision 2023)
+++ kern/disk.c	(working copy)
@@ -157,11 +175,14 @@ grub_disk_cache_store (unsigned long dev_id, unsig
 {
   unsigned index;
   struct grub_disk_cache *cache;
-  
-  grub_disk_cache_invalidate (dev_id, disk_id, sector);
  
   index = grub_disk_cache_get_index (dev_id, disk_id, sector);
   cache = grub_disk_cache_table + index;
+
+  cache->lock = 1;
+  grub_free (cache->data);
+  cache->data = 0;
+  cache->lock = 0;
   
   cache->data = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
   if (! cache->data)
Index: disk/scsi.c
===================================================================
--- disk/scsi.c	(revision 2023)
+++ disk/scsi.c	(working copy)
@@ -255,6 +255,7 @@ grub_scsi_open (const char *name, grub_disk_t disk
 	  scsi->name = grub_strdup (name);
 	  if (! scsi->name)
 	    {
+	      grub_free (scsi);
 	      return grub_errno;
 	    }
 
@@ -263,6 +264,7 @@ grub_scsi_open (const char *name, grub_disk_t disk
 	  err = grub_scsi_inquiry (scsi);
 	  if (err)
 	    {
+	      grub_free (scsi);
 	      grub_dprintf ("scsi", "inquiry failed\n");
 	      return grub_errno;
 	    }
@@ -275,6 +277,7 @@ grub_scsi_open (const char *name, grub_disk_t disk
 	  if (scsi->devtype != grub_scsi_devtype_direct
 	      && scsi->devtype != grub_scsi_devtype_cdrom)
 	    {
+	      grub_free (scsi);
 	      return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
 				 "unknown SCSI device");
 	    }
@@ -287,6 +290,7 @@ grub_scsi_open (const char *name, grub_disk_t disk
 	  err = grub_scsi_read_capacity (scsi);
 	  if (err)
 	    {
+	      grub_free (scsi);
 	      grub_dprintf ("scsi", "READ CAPACITY failed\n");
 	      return grub_errno;
 	    }
@@ -303,6 +307,8 @@ grub_scsi_open (const char *name, grub_disk_t disk
 	}
     }
 
+  grub_free (scsi);
+
   return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a SCSI disk");
 }
 
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 2023)
+++ ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2009-03-09  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Leak fixes
+
+	* kern/disk.c (grub_disk_cache_store): Invalidate previous cache 
+	in case of collision
+	* disk/scsi.c (grub_scsi_open): free scsi in case of error
+
 2009-03-09  Felix Zielcke  <fzielcke@z-51.de>
 
 	* conf/powerpc-ieee1275.rmk (grub_emu_SOURCES): Remove duplicated

Re: Leak fixes

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

Rediffed
2009-03-22  Vladimir Serbinenko  <phcoder@gmail.com>

       Leak fixes

       * kern/disk.c (grub_disk_cache_store): Invalidate previous cache
       in case of collision
       * disk/scsi.c (grub_scsi_open): free scsi in case of error


phcoder wrote:
> Hello I discovered some memory leaks. Here is the fix
> The main one was is that if a hash collision occurs new cache entry 
> overwrites the old one but the old one wasn't freed. When loading a big 
> file it was leaking huge amounts of memory
> 


-- 

Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: leakfixes.diff --]
[-- Type: text/x-patch, Size: 1894 bytes --]

diff --git a/disk/scsi.c b/disk/scsi.c
index 75b92b4..319d8d6 100644
--- a/disk/scsi.c
+++ b/disk/scsi.c
@@ -255,6 +255,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 	  scsi->name = grub_strdup (name);
 	  if (! scsi->name)
 	    {
+	      grub_free (scsi);
 	      return grub_errno;
 	    }
 
@@ -263,6 +264,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 	  err = grub_scsi_inquiry (scsi);
 	  if (err)
 	    {
+	      grub_free (scsi);
 	      grub_dprintf ("scsi", "inquiry failed\n");
 	      return grub_errno;
 	    }
@@ -275,6 +277,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 	  if (scsi->devtype != grub_scsi_devtype_direct
 	      && scsi->devtype != grub_scsi_devtype_cdrom)
 	    {
+	      grub_free (scsi);
 	      return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
 				 "unknown SCSI device");
 	    }
@@ -287,6 +290,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 	  err = grub_scsi_read_capacity (scsi);
 	  if (err)
 	    {
+	      grub_free (scsi);
 	      grub_dprintf ("scsi", "READ CAPACITY failed\n");
 	      return grub_errno;
 	    }
@@ -303,6 +307,8 @@ grub_scsi_open (const char *name, grub_disk_t disk)
 	}
     }
 
+  grub_free (scsi);
+
   return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a SCSI disk");
 }
 
diff --git a/kern/disk.c b/kern/disk.c
index 4ee03e0..8a92989 100644
--- a/kern/disk.c
+++ b/kern/disk.c
@@ -158,10 +158,13 @@ grub_disk_cache_store (unsigned long dev_id, unsigned long disk_id,
   unsigned index;
   struct grub_disk_cache *cache;
   
-  grub_disk_cache_invalidate (dev_id, disk_id, sector);
-  
   index = grub_disk_cache_get_index (dev_id, disk_id, sector);
   cache = grub_disk_cache_table + index;
+ 
+  cache->lock = 1;
+  grub_free (cache->data);
+  cache->data = 0;
+  cache->lock = 0;
   
   cache->data = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
   if (! cache->data)

Re: Leak fixes

[Robert Millan]

On Sun, Mar 22, 2009 at 02:09:52PM +0100, phcoder wrote:
>    
> -  grub_disk_cache_invalidate (dev_id, disk_id, sector);
> -  
>    index = grub_disk_cache_get_index (dev_id, disk_id, sector);
>    cache = grub_disk_cache_table + index;
> + 
> +  cache->lock = 1;
> +  grub_free (cache->data);
> +  cache->data = 0;
> +  cache->lock = 0;

Does this imply grub_disk_cache_invalidate() is not working properly?  Can it
be fixed instead?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Leak fixes

[phcoder]

Robert Millan wrote:
> On Sun, Mar 22, 2009 at 02:09:52PM +0100, phcoder wrote:
>>    
>> -  grub_disk_cache_invalidate (dev_id, disk_id, sector);
>> -  
>>    index = grub_disk_cache_get_index (dev_id, disk_id, sector);
>>    cache = grub_disk_cache_table + index;
>> + 
>> +  cache->lock = 1;
>> +  grub_free (cache->data);
>> +  cache->data = 0;
>> +  cache->lock = 0;
> 
> Does this imply grub_disk_cache_invalidate() is not working properly?  Can it
> be fixed instead?
> 
No it works fine. But it's used to say to cache subsystem "block number 
N on disk DISK" was updated. In this case when new block collides with 
old one old one has to be freed even if it's still valid

-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: Leak fixes

[phcoder]

committed
phcoder wrote:
> Robert Millan wrote:
>> On Sun, Mar 22, 2009 at 02:09:52PM +0100, phcoder wrote:
>>>    -  grub_disk_cache_invalidate (dev_id, disk_id, sector);
>>> -     index = grub_disk_cache_get_index (dev_id, disk_id, sector);
>>>    cache = grub_disk_cache_table + index;
>>> + +  cache->lock = 1;
>>> +  grub_free (cache->data);
>>> +  cache->data = 0;
>>> +  cache->lock = 0;
>>
>> Does this imply grub_disk_cache_invalidate() is not working properly?  
>> Can it
>> be fixed instead?
>>
> No it works fine. But it's used to say to cache subsystem "block number 
> N on disk DISK" was updated. In this case when new block collides with 
> old one old one has to be freed even if it's still valid
> 


-- 

Regards
Vladimir 'phcoder' Serbinenko