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

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

[boehm.igor]

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;

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

[Noam Preil]

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;

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

[Noam Preil]

Gah, apologies for top posting, thought I had deleted the quoted bits.

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

[Noam Preil]

I should also check the source instead of relying upon the email, sorry.

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

[boehm.igor]

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