[patch] target_read_string, potential memory leak.

[patch] target_read_string, potential memory leak.

[msnyder]

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

OK -- it would be pretty stupid to call target_read_string without
passing it a buffer, but if we're going to check it for null...


[-- Attachment #2: 137.txt --]
[-- Type: text/plain, Size: 567 bytes --]

2007-08-04  Michael Snyder  <msnyder@access-company.com>

	* target.c (target_read_string): Potential memory leak.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.145
diff -p -r1.145 target.c
*** target.c	24 Jul 2007 12:49:24 -0000	1.145
--- target.c	5 Aug 2007 03:25:25 -0000
*************** done:
*** 972,977 ****
--- 972,980 ----
      *errnop = errcode;
    if (string != NULL)
      *string = buffer;
+   else
+     xfree (buffer);
+ 
    return nbytes_read;
  }
  

Re: [patch] target_read_string, potential memory leak.

[Jim Blandy]

msnyder@sonic.net writes:
> OK -- it would be pretty stupid to call target_read_string without
> passing it a buffer, but if we're going to check it for null...

I don't think the contract for target_read_string permits STRING to be
null.  I think either the test should go, or we should add a
gdb_assert (string).

Re: [patch] target_read_string, potential memory leak.

[msnyder]

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

>
> msnyder@sonic.net writes:
>> OK -- it would be pretty stupid to call target_read_string without
>> passing it a buffer, but if we're going to check it for null...
>
> I don't think the contract for target_read_string permits STRING to be
> null.  I think either the test should go, or we should add a
> gdb_assert (string).

Agreed.  Thus?


[-- Attachment #2: target_string.txt --]
[-- Type: text/plain, Size: 939 bytes --]

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.145
diff -p -r1.145 target.c
*** target.c	24 Jul 2007 12:49:24 -0000	1.145
--- target.c	8 Aug 2007 18:31:57 -0000
*************** target_read_string (CORE_ADDR memaddr, c
*** 919,924 ****
--- 919,926 ----
    char *bufptr;
    unsigned int nbytes_read = 0;
  
+   gdb_assert (string);
+ 
    /* Small for testing.  */
    buffer_allocated = 4;
    buffer = xmalloc (buffer_allocated);
*************** target_read_string (CORE_ADDR memaddr, c
*** 968,977 ****
        nbytes_read += tlen;
      }
  done:
    if (errnop != NULL)
      *errnop = errcode;
-   if (string != NULL)
-     *string = buffer;
    return nbytes_read;
  }
  
--- 970,978 ----
        nbytes_read += tlen;
      }
  done:
+   *string = buffer;
    if (errnop != NULL)
      *errnop = errcode;
    return nbytes_read;
  }
  

Re: [patch] target_read_string, potential memory leak.

[Jim Blandy]

msnyder@sonic.net writes:
>> msnyder@sonic.net writes:
>>> OK -- it would be pretty stupid to call target_read_string without
>>> passing it a buffer, but if we're going to check it for null...
>>
>> I don't think the contract for target_read_string permits STRING to be
>> null.  I think either the test should go, or we should add a
>> gdb_assert (string).
>
> Agreed.  Thus?

Agreed, thus.  :)

> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.145
> diff -p -r1.145 target.c
> *** target.c	24 Jul 2007 12:49:24 -0000	1.145
> --- target.c	8 Aug 2007 18:31:57 -0000
> *************** target_read_string (CORE_ADDR memaddr, c
> *** 919,924 ****
> --- 919,926 ----
>     char *bufptr;
>     unsigned int nbytes_read = 0;
>   
> +   gdb_assert (string);
> + 
>     /* Small for testing.  */
>     buffer_allocated = 4;
>     buffer = xmalloc (buffer_allocated);
> *************** target_read_string (CORE_ADDR memaddr, c
> *** 968,977 ****
>         nbytes_read += tlen;
>       }
>   done:
>     if (errnop != NULL)
>       *errnop = errcode;
> -   if (string != NULL)
> -     *string = buffer;
>     return nbytes_read;
>   }
>   
> --- 970,978 ----
>         nbytes_read += tlen;
>       }
>   done:
> +   *string = buffer;
>     if (errnop != NULL)
>       *errnop = errcode;
>     return nbytes_read;
>   }
>   

Re: [patch] target_read_string, potential memory leak.

[msnyder]

>
> msnyder@sonic.net writes:
>>> msnyder@sonic.net writes:
>>>> OK -- it would be pretty stupid to call target_read_string without
>>>> passing it a buffer, but if we're going to check it for null...
>>>
>>> I don't think the contract for target_read_string permits STRING to be
>>> null.  I think either the test should go, or we should add a
>>> gdb_assert (string).
>>
>> Agreed.  Thus?
>
> Agreed, thus.  :)

Committed.  Fiat.