Re: [9front] cmd/acme: fix resource leak in ecmd.c (patch)

[Noam Preil <noam@pixelhero.dev>]

Uh, there is `r = list`.

Feb 1, 2021 18:15:16 boehm.igor@gmail.com:

> There is a resource leak in acme functions:
> • /sys/src/cmd/acme/ecmd.c:/^B_cmd
> • /sys/src/cmd/acme/ecmd.c:/^D_cmd
>
> I will describe the case for function B_cmd, the issue is the same for
> D_cmd.
>
> In short, there can be a case where the heap allocated storage
> returned by the /sys/src/cmd/acme/ecmd.c:/^filelist(...) function is
> not freed. You can use the following as a starting point:
>
> • /sys/src/cmd/acme/ecmd.c:192
>
> int
> B_cmd(Text *t, Cmd *cp)
> {
>   Rune *list, *r, *s;
>   int nr;
>
>   list = filelist(t, cp->text->r, cp->text->n);
>   ^^^^^^^^^^^^^^^
>   if(list == nil)
>     editerror(Enoname);
>   r = list;
>   ...
>   ...
>   }
>   clearcollection();
>   ^^^^^^^^^^^^^^^^^
>   return TRUE;
> }
>
> Notice the two lines that are highlighted, namely the call to
> filelist(...) and clearcollection(...).
>
> Now if we look at the impl of filelist(...) we have:
>
> /* string is known to be NUL-terminated */
> Rune*
> filelist(Text *t, Rune *r, int nr)
> {
>   if(nr == 0)
>     return nil;
>   r = skipbl(r, nr, &nr);
>   if(r[0] != '<')
>     return runestrdup(r);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> WHEN WE TAKE THE ABOVE PATH WE LEAK
> WHAT runestrdup(r) RETURNS
>   /* use < command to collect text */
>   clearcollection();
>   runpipe(t, '<', r+1, nr-1, Collecting);
>   return collection;
> ^^^^^^^^^^^^^^^^^^^^^^
> TAKING THIS PATH IS FINE AS clearcollection(...)
> WILL FREE ANY HEAP ALLOCATED MEMORY
> }
>
> So for the case where we *do not* return the 'collection' pointer we
> leak memory that is allocated by 'runestrdup(...)'.
>
> There are probably more elegant solutions for this.  The below simply
> checks if the pointer returned by filelist(...) is not equal to
> 'collection'.  That is used as a sentinel to detect the case where
> runestrdup(...) has been called.  That is the case where the memory
> returned by filelist(...) is *not* freed by clearcollection(...) and
> requires an extra free.
>
> Somehow the explanation is much more complicated than I initially
> intended.  Hope you can still follow the reasoning and I haven't
> screwed up somewhere.
>
> Here is the patch:
>
> diff -r 0b8c8ef6a3d4 sys/src/cmd/acme/ecmd.c
> --- a/sys/src/cmd/acme/ecmd.c Tue Jan 19 15:18:57 2021 -0800
> +++ b/sys/src/cmd/acme/ecmd.c Mon Feb 01 22:24:56 2021 +0100
> @@ -204,6 +204,8 @@
>     if(nr > 0)
>       r = skipbl(s+1, nr-1, &nr);
>   }
> + if(list != collection)
> +   free(list);
>   clearcollection();
>   return TRUE;
> }
> @@ -278,6 +280,8 @@
>     if(nr > 0)
>       r = skipbl(s+1, nr-1, &nr);
>   }while(nr > 0);
> + if(list != collection)
> +   free(list);
>   clearcollection();
>   free(dir.r);
>   return TRUE;