PATCH: readline/histexpand.c, resource leak

PATCH: readline/histexpand.c, resource leak

[msnyder]

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



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

2007-07-31  Michael Snyder  <msnyder@access-company.com>

	* histexpand.c (history_find_word): Resource leak.

Index: histexpand.c
===================================================================
RCS file: /cvs/src/src/readline/histexpand.c,v
retrieving revision 1.6
diff -p -r1.6 histexpand.c
*** histexpand.c	5 May 2006 18:26:12 -0000	1.6
--- histexpand.c	1 Aug 2007 02:08:51 -0000
*************** history_find_word (line, ind)
*** 1581,1588 ****
    int i, wind;
  
    words = history_tokenize_internal (line, ind, &wind);
!   if (wind == -1 || words == 0)
      return ((char *)NULL);
    s = words[wind];
    for (i = 0; i < wind; i++)
      free (words[i]);
--- 1581,1594 ----
    int i, wind;
  
    words = history_tokenize_internal (line, ind, &wind);
!   if (words == NULL)
      return ((char *)NULL);
+   if (wind == -1)
+     {
+       free (words);
+       return ((char *)NULL);
+     }
+ 
    s = words[wind];
    for (i = 0; i < wind; i++)
      free (words[i]);

Re: PATCH: readline/histexpand.c, resource leak

[msnyder]

>
Ping?

Re: PATCH: readline/histexpand.c, resource leak

[Jim Blandy]

It seems to me that 'words' is a malloc'ed array of malloc'ed strings,
so 'free (words)' still leaks storage.

msnyder@sonic.net writes:
> 2007-07-31  Michael Snyder  <msnyder@access-company.com>
>
> 	* histexpand.c (history_find_word): Resource leak.
>
> Index: histexpand.c
> ===================================================================
> RCS file: /cvs/src/src/readline/histexpand.c,v
> retrieving revision 1.6
> diff -p -r1.6 histexpand.c
> *** histexpand.c	5 May 2006 18:26:12 -0000	1.6
> --- histexpand.c	1 Aug 2007 02:08:51 -0000
> *************** history_find_word (line, ind)
> *** 1581,1588 ****
>     int i, wind;
>   
>     words = history_tokenize_internal (line, ind, &wind);
> !   if (wind == -1 || words == 0)
>       return ((char *)NULL);
>     s = words[wind];
>     for (i = 0; i < wind; i++)
>       free (words[i]);
> --- 1581,1594 ----
>     int i, wind;
>   
>     words = history_tokenize_internal (line, ind, &wind);
> !   if (words == NULL)
>       return ((char *)NULL);
> +   if (wind == -1)
> +     {
> +       free (words);
> +       return ((char *)NULL);
> +     }
> + 
>     s = words[wind];
>     for (i = 0; i < wind; i++)
>       free (words[i]);

Re: PATCH: readline/histexpand.c, resource leak

[msnyder]

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

>
> It seems to me that 'words' is a malloc'ed array of malloc'ed strings,
> so 'free (words)' still leaks storage.

Yeaaaahh... maybe the attached is better?  Or is this use
of goto a little opaque?

By the way, are we maintaining our own readline fork?
Should I check these in locally?


[-- Attachment #2: 130b.txt --]
[-- Type: text/plain, Size: 1107 bytes --]

Index: histexpand.c
===================================================================
RCS file: /cvs/src/src/readline/histexpand.c,v
retrieving revision 1.6
diff -p -r1.6 histexpand.c
*** histexpand.c	5 May 2006 18:26:12 -0000	1.6
--- histexpand.c	15 Aug 2007 00:37:32 -0000
*************** history_find_word (line, ind)
*** 1577,1591 ****
       char *line;
       int ind;
  {
!   char **words, *s;
    int i, wind;
  
    words = history_tokenize_internal (line, ind, &wind);
!   if (wind == -1 || words == 0)
!     return ((char *)NULL);
    s = words[wind];
    for (i = 0; i < wind; i++)
      free (words[i]);
    for (i = wind + 1; words[i]; i++)
      free (words[i]);
    free (words);
--- 1577,1595 ----
       char *line;
       int ind;
  {
!   char **words, *s = NULL;
    int i, wind;
  
    words = history_tokenize_internal (line, ind, &wind);
!   if (words == NULL)
!     return NULL;
!   if (wind == -1)
!     goto bail;
! 
    s = words[wind];
    for (i = 0; i < wind; i++)
      free (words[i]);
+  bail:
    for (i = wind + 1; words[i]; i++)
      free (words[i]);
    free (words);

Re: PATCH: readline/histexpand.c, resource leak

[Daniel Jacobowitz]

On Tue, Aug 14, 2007 at 05:37:47PM -0700, Michael Snyder wrote:
> By the way, are we maintaining our own readline fork?
> Should I check these in locally?

No, we are not.  Or rather, we sort of are, but we're trying not to.
I believe the only patches in our version are build related and DOS
and/or Windows related ones that I didn't really grok; everything else
has been merged.

Personally, I would be happy to remove the directory entirely, but I'm
sure that would inconvenience some people.  I use the system readline
to build Debian GDB packages.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: readline/histexpand.c, resource leak

[msnyder]

> On Tue, Aug 14, 2007 at 05:37:47PM -0700, Michael Snyder wrote:
>> By the way, are we maintaining our own readline fork?
>> Should I check these in locally?
>
> No, we are not.  Or rather, we sort of are, but we're trying not to.
> I believe the only patches in our version are build related and DOS
> and/or Windows related ones that I didn't really grok; everything else
> has been merged.
>
> Personally, I would be happy to remove the directory entirely, but I'm
> sure that would inconvenience some people.  I use the system readline
> to build Debian GDB packages.

OK, well I heard from Chet Ramey to the effect that he had already
checked these in to his sources (or at least, the parts of them that
he wanted) -- so I guess I'll just let it go at that until the next
time we merge (or whatever we do).

Re: PATCH: readline/histexpand.c, resource leak

[Jim Blandy]

msnyder@sonic.net writes:
> Yeaaaahh... maybe the attached is better?  Or is this use
> of goto a little opaque?

Personally, I'd add a new function 'free_history_words' and call it.

Re: [Bug-readline] Re: PATCH: readline/histexpand.c, resource leak

[Chet Ramey]

Jim Blandy wrote:
> It seems to me that 'words' is a malloc'ed array of malloc'ed strings,
> so 'free (words)' still leaks storage.

I don't believe there's a case where wind == -1 and any malloced strings
exist in words[].  There's no leak unless that can happen.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [Bug-readline] Re: PATCH: readline/histexpand.c, resource leak

[msnyder]

> Jim Blandy wrote:
>> It seems to me that 'words' is a malloc'ed array of malloc'ed strings,
>> so 'free (words)' still leaks storage.
>
> I don't believe there's a case where wind == -1 and any malloced strings
> exist in words[].  There's no leak unless that can happen.

Well, that isn't locally obvious -- it depends on knowing the
behavior of other code.

You may be right -- but the free won't do any harm, and in case
you're wrong (now or in the future), the free will catch it.

Re: [Bug-readline] Re: PATCH: readline/histexpand.c, resource leak

[Jim Blandy]

Chet Ramey <chet.ramey@case.edu> writes:
> Jim Blandy wrote:
>> It seems to me that 'words' is a malloc'ed array of malloc'ed strings,
>> so 'free (words)' still leaks storage.
>
> I don't believe there's a case where wind == -1 and any malloced strings
> exist in words[].  There's no leak unless that can happen.

Not even if history_tokenize_internal's 'windp' argument falls in
whitespace between words?

Re: [Bug-readline] Re: PATCH: readline/histexpand.c, resource leak

[Chet Ramey]

Jim Blandy wrote:

>>> It seems to me that 'words' is a malloc'ed array of malloc'ed strings,
>>> so 'free (words)' still leaks storage.
>> I don't believe there's a case where wind == -1 and any malloced strings
>> exist in words[].  There's no leak unless that can happen.
> 
> Not even if history_tokenize_internal's 'windp' argument falls in
> whitespace between words?

Yes, I think you're right.  Thanks.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/