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

[boehm.igor@gmail.com]

Sorry, I have not provided a test case for when the leak happens.

To trigger the leak type the following into an acme Tag (assuming
sam.save exists):

• Edit B sam.save

and execute it (i.e. select and middle click). This is the case where
memory is leaked.

If the input to the B command comes from something preceded with '<'
there is no leak.  So for this case:

• Edit B <ls

Things are freed up properly (see
/sys/src/cmd/acme/ecmd.c:/^filelist(...)).  Don't run this in a folder
with many files or folders as it will open window for each file and
folder ☺

So if you open man files/folders using the B command and provide the
explicit file name or delete them using the D command, you can watch
leaked memory accumulate.

Small side note:

The 'Edit B <ls' is a really neat way to open all files and folders in
the current folder quickly.  Spinning this further one could envisage
a script that given a file name will search all .h and .c files
containing a string and open them, etc.

HTH

Cheers,
Igor


Quoth 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;
>