9front - general discussion about 9front
 help / color / mirror / Atom feed
From: igor@9lab.org
To: 9front@9front.org
Cc: igor@9lab.org
Subject: [9front] cmd/rc: fix leaking runq->cmdfile (patch)
Date: Tue, 09 Nov 2021 13:45:45 +0100	[thread overview]
Message-ID: <2203D6ED28C225EDCF76C0393E1960A8@9lab.org> (raw)

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

The below patch is for review and testing. It plugs a memory leak in rc.

Please give this a try and/or provide feedback if you run into any issues
or disagree with the solution. If there is agreement and no one runs into
problems over the next week or two (I have been running with this patch for
a while on a a few machines) I would like to merge this and start looking
at the other leaks in rc…

Here the git/importable patch inline for reading (it is attached as well
to applying/testing):

<snip>
From: Igor Böhm <igor@9lab.org>
Date: Tue, 09 Nov 2021 12:33:08 +0000
Subject: [PATCH] cmd/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 338
<stdin>:2: (error) mainmem used but not set
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 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 338
<stdin>:2: (error) mainmem used but not set
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>

...fixing those will hopefully follow soon.
---
diff a7ec6ee4e8103bb448f902e9b6d0dc69629abc57 0e2fed4e4b042109f86ee6006ecdaa63077c669c
--- a/sys/src/cmd/rc/exec.c	Mon Nov  8 02:05:51 2021
+++ b/sys/src/cmd/rc/exec.c	Tue Nov  9 13:33:08 2021
@@ -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();
 		}

</snap>

[-- Attachment #2: rc-fix-thread.cmdfile-leak.patch --]
[-- Type: text/plain, Size: 4149 bytes --]

From: Igor Böhm <igor@9lab.org>
Date: Tue, 09 Nov 2021 12:33:08 +0000
Subject: [PATCH] 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 (capture 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 338
<stdin>:2: (error) mainmem used but not set
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 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 338
<stdin>:2: (error) mainmem used but not set
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>

...fixing those will hopefully follow soon.
---
diff a7ec6ee4e8103bb448f902e9b6d0dc69629abc57 0e2fed4e4b042109f86ee6006ecdaa63077c669c
--- a/sys/src/cmd/rc/exec.c	Mon Nov  8 02:05:51 2021
+++ b/sys/src/cmd/rc/exec.c	Tue Nov  9 13:33:08 2021
@@ -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();
 		}

             reply	other threads:[~2021-11-09 13:06 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-09 12:45 igor [this message]
2021-11-09 19:27 ` cinap_lenrek
2021-11-10 13:21   ` igor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2203D6ED28C225EDCF76C0393E1960A8@9lab.org \
    --to=igor@9lab.org \
    --cc=9front@9front.org \
    --subject='Re: [9front] cmd/rc: fix leaking runq->cmdfile (patch)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).