shithub: riscv

Download patch

ref: c7775b365ef3e73748f23b7ace521214753de2a7
parent: a4c1f3cc18df6fddd548f4df9f209695c4eb7263
author: Igor Böhm <igor@9lab.org>
date: Wed Nov 10 08:01:38 EST 2021

rc: fix leaking runq->cmdfile

To reproduce run the following on a terminal:
<snip>
cpu% leak -s `{pstree | grep termrc | sed 1q | awk '{print $1}'}
src(0x00209a82); // 12
src(0x0020b2a6); // 1
cpu% acid `{pstree | grep termrc | sed 1q | awk '{print $1}'}
/proc/358/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: src(0x0020b2a6)
/sys/src/cmd/rc/plan9.c:169
 164		if(runq->argv->words == 0)
 165			poplist();
 166		else {
 167			free(runq->cmdfile);
 168			int f = open(runq->argv->words->word, 0);
>169			runq->cmdfile = strdup(runq->argv->words->word);
 170			runq->lexline = 1;
 171			runq->pc--;
 172			popword();
 173			if(f>=0) execcmds(openfd(f));
 174		}
acid:
</snap>

Another `runq->cmdfile` leak is present here (captured on a cpu server):
<snip>
277         ├listen [tcp * /rc/bin/service <nil>]
321         │├listen [/net/tcp/2 tcp!*!80]
322         │├listen [/net/tcp/3 tcp!*!17019]
324         ││└rc [/net/tcp/5 tcp!185.64.155.70!3516]
334         ││ ├rc -li
382         ││ │└pstree
336         ││ └rc
338         ││  └cat
323         │└listen [/net/tcp/4 tcp!*!17020]
278         ├listen [tcp * /rc/bin/service.auth <nil>]
320         │└listen [/net/tcp/1 tcp!*!567]
381         └closeproc
cpu% leak -s 336
src(0x00209a82); // 2
src(0x002051d2); // 1
cpu% acid 336
/proc/336/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: src(0x002051d2)
/sys/src/cmd/rc/exec.c:1056
 1051
 1052	void
 1053	Xsrcfile(void)
 1054	{
 1055		free(runq->cmdfile);
>1056		runq->cmdfile = strdup(runq->code[runq->pc++].s);
 1057	}
acid:
</snap>

These leaks happen because we do not free cmdfile on all execution paths
where `Xreturn()` is invoked. In `/sys/src/cmd/rc/exec.c:/^Xreturn`

<snip>
void
Xreturn(void)
{
	struct thread *p = runq;
	turfredir();
	while(p->argv) poplist();
	codefree(p->code);
	runq = p->ret;
	free(p);
	if(runq==0)
		Exit(getstatus());
}
</snip>

Note how the function `Xreturn()` frees a heap allocated instance of type
`thread` with its members *except* the `cmdfile` member.

On some code paths where `Xreturn()` is called there is an attempt to free
`cmdfile`, however, there are some code paths where `Xreturn()` is called
where `cmdfile` is not freed, leading to a leak.

The attached patch calls `free(p->cmdfile)` in `Xreturn()` to avoid leaking
memory and handling the free in one place.

After applying the patch this particular leak is removed. There are still
other leaks in rc:

<snip>
277         ├listen [tcp * /rc/bin/service <nil>]
321         │├listen [/net/tcp/2 tcp!*!80]
322         │├listen [/net/tcp/3 tcp!*!17019]
324         ││└rc [/net/tcp/5 tcp!185.64.155.70!3516]
334         ││ ├rc -li
382         ││ │└pstree
336         ││ └rc
338         ││  └cat
323         │└listen [/net/tcp/4 tcp!*!17020]
278         ├listen [tcp * /rc/bin/service.auth <nil>]
320         │└listen [/net/tcp/1 tcp!*!567]
381         └closeproc
cpu% leak -s 336
src(0x00209a82); // 2
src(0x002051d2); // 1
cpu% acid 336
/proc/336/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: src(0x00209a82)
/sys/src/cmd/rc/subr.c:9
 4	#include "fns.h"
 5
 6	void *
 7	emalloc(long n)
 8	{
>9		void *p = malloc(n);
 10		if(p==0)
 11			panic("Can't malloc %d bytes", n);
 12		return p;
 13	}
 14
</snap>

To help fixing those leaks emalloc(…) and erealloc(…) have been amended to use
setmalloctag(…) and setrealloctag(…). The actual fixes for other reported leaks
are *not* part of this merge and will follow.

--- a/sys/src/cmd/rc/exec.c
+++ b/sys/src/cmd/rc/exec.c
@@ -465,6 +465,7 @@
 	turfredir();
 	while(p->argv) poplist();
 	codefree(p->code);
+	free(p->cmdfile);
 	runq = p->ret;
 	free(p);
 	if(runq==0)
@@ -937,8 +938,6 @@
 	Noerror();
 	if(yyparse()){
 		if(!p->iflag || p->eof && !Eintr()){
-			if(p->cmdfile)
-				free(p->cmdfile);
 			closeio(p->cmdfd);
 			Xreturn();
 		}
--- a/sys/src/cmd/rc/subr.c
+++ b/sys/src/cmd/rc/subr.c
@@ -9,6 +9,7 @@
 	void *p = malloc(n);
 	if(p==0)
 		panic("Can't malloc %d bytes", n);
+	setmalloctag(p, getcallerpc(&n));
 	return p;
 }
 
@@ -18,6 +19,7 @@
 	p = realloc(p, n);
 	if(p==0 && n!=0)
 		panic("Can't realloc %d bytes\n", n);
+	setrealloctag(p, getcallerpc(&p));
 	return p;
 }