[patch] valprint.c (read_string): Rework clean-up logic.

[patch] valprint.c (read_string): Rework clean-up logic.

[Phil Muldoon]

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

Hi,

While performing some Valgrind profiling on GDB we found that the 
"read_string" function in valprint.c can leak clean-ups.  This happens 
as it does not account for a previous clean-up in the local chain and 
overwrites the local "old_chain" place holder with a new one. This patch 
reworks the function's clean-up logic, only registering one clean-up at 
the beginning and including a new clean-up function. This new clean-up 
function frees buffers referenced by double indirection.

This patch neither increases or decreases regressions in the test-suite. 
It was built and tested on X86_64.

2009-02-27  Phil Muldoon  <pmuldoon@redhat.com>

    * valprint.c (xfree_gdb_byte): New function.
    (read_string): Rework clean-up logic.  Use xfree_gdb_byte for
    clean-up.



[-- Attachment #2: read_string_cleanup-cvs.patch --]
[-- Type: text/x-patch, Size: 1844 bytes --]

Index: gdb/valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/valprint.c,v
retrieving revision 1.79
diff -u -r1.79 valprint.c
--- gdb/valprint.c	5 Feb 2009 12:16:25 -0000	1.79
+++ gdb/valprint.c	27 Feb 2009 17:55:46 -0000
@@ -1177,6 +1177,15 @@
   return (nread);
 }
 
+/* Local clean-up for a gdb_byte buffer pointed to via double
+   indirection.  This function is used in read_string.  */
+static void
+xfree_gdb_byte (void *buffer)
+{
+  gdb_byte **pointer = buffer;
+  xfree (*pointer);
+}
+
 /* Read a string from the inferior, at ADDR, with LEN characters of WIDTH bytes
    each.  Fetch at most FETCHLIMIT characters.  BUFFER will be set to a newly
    allocated buffer containing the string, which the caller is responsible to
@@ -1226,13 +1235,14 @@
      some error, such as bumping into the end of the address space.  */
 
   found_nul = 0;
-  old_chain = make_cleanup (null_cleanup, 0);
+  *buffer = NULL;
+
+  old_chain = make_cleanup (xfree_gdb_byte, buffer);
 
   if (len > 0)
     {
       *buffer = (gdb_byte *) xmalloc (len * width);
       bufptr = *buffer;
-      old_chain = make_cleanup (xfree, *buffer);
 
       nfetch = partial_memory_read (addr, bufptr, len * width, &errcode)
 	/ width;
@@ -1243,8 +1253,6 @@
     {
       unsigned long bufsize = 0;
 
-      *buffer = NULL;
-
       do
 	{
 	  QUIT;
@@ -1253,13 +1261,9 @@
 	  if (*buffer == NULL)
 	    *buffer = (gdb_byte *) xmalloc (nfetch * width);
 	  else
-	    {
-	      discard_cleanups (old_chain);
-	      *buffer = (gdb_byte *) xrealloc (*buffer,
-					       (nfetch + bufsize) * width);
-	    }
+	    *buffer = (gdb_byte *) xrealloc (*buffer,
+					     (nfetch + bufsize) * width);
 
-	  old_chain = make_cleanup (xfree, *buffer);
 	  bufptr = *buffer + bufsize * width;
 	  bufsize += nfetch;
 

Re: [patch] valprint.c (read_string): Rework clean-up logic.

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> 2009-02-27  Phil Muldoon  <pmuldoon@redhat.com>
Phil>    * valprint.c (xfree_gdb_byte): New function.
Phil>    (read_string): Rework clean-up logic.  Use xfree_gdb_byte for
Phil>    clean-up.

This is almost ok...

Phil> +/* Local clean-up for a gdb_byte buffer pointed to via double
Phil> +   indirection.  This function is used in read_string.  */
Phil> +static void
Phil> +xfree_gdb_byte (void *buffer)

I forgot that there is already a function for this --
free_current_contents.  So, this new function is not needed...

Phil> +  old_chain = make_cleanup (xfree_gdb_byte, buffer);
 
... and you can just s/xfree_gdb_byte/free_current_contents/ here.

It is ok with that change.  Thanks.

Tom

Re: [patch] valprint.c (read_string): Rework clean-up logic.

[Phil Muldoon]

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

Tom Tromey wrote:
>
> Phil> +  old_chain = make_cleanup (xfree_gdb_byte, buffer);
>  
> ... and you can just s/xfree_gdb_byte/free_current_contents/ here.
>
> It is ok with that change.  Thanks.
>
> Tom
>   
Done. I attached the modified patch for reference.

[-- Attachment #2: read_string_cleanup-cvs2.patch --]
[-- Type: text/x-patch, Size: 1351 bytes --]

Index: valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/valprint.c,v
retrieving revision 1.79
retrieving revision 1.80
diff -u -r1.79 -r1.80
--- valprint.c	5 Feb 2009 12:16:25 -0000	1.79
+++ valprint.c	27 Feb 2009 19:33:06 -0000	1.80
@@ -1226,13 +1226,14 @@
      some error, such as bumping into the end of the address space.  */
 
   found_nul = 0;
-  old_chain = make_cleanup (null_cleanup, 0);
+  *buffer = NULL;
+
+  old_chain = make_cleanup (free_current_contents, buffer);
 
   if (len > 0)
     {
       *buffer = (gdb_byte *) xmalloc (len * width);
       bufptr = *buffer;
-      old_chain = make_cleanup (xfree, *buffer);
 
       nfetch = partial_memory_read (addr, bufptr, len * width, &errcode)
 	/ width;
@@ -1243,8 +1244,6 @@
     {
       unsigned long bufsize = 0;
 
-      *buffer = NULL;
-
       do
 	{
 	  QUIT;
@@ -1253,13 +1252,9 @@
 	  if (*buffer == NULL)
 	    *buffer = (gdb_byte *) xmalloc (nfetch * width);
 	  else
-	    {
-	      discard_cleanups (old_chain);
-	      *buffer = (gdb_byte *) xrealloc (*buffer,
-					       (nfetch + bufsize) * width);
-	    }
+	    *buffer = (gdb_byte *) xrealloc (*buffer,
+					     (nfetch + bufsize) * width);
 
-	  old_chain = make_cleanup (xfree, *buffer);
 	  bufptr = *buffer + bufsize * width;
 	  bufsize += nfetch;