9front - general discussion about 9front
 help / color / mirror / Atom feed
From: igor@9lab.org
To: 9front@9front.org
Cc: igor@9lab.org
Subject: Re: [9front] cmd/rc: fix leaking runq->cmdfile (patch)
Date: Wed, 10 Nov 2021 14:21:54 +0100	[thread overview]
Message-ID: <783E98DBC7A1EBAD7AB87A2E5E0673D0@9lab.org> (raw)
In-Reply-To: <316DC1762EF8EB5DC3E1756D964999CD@felloff.net>

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

Quoth cinap_lenrek@felloff.net:
> nice, looks good to me on first glance.
> 
> note, we should probably add a setmalloctag(p, getcallerpc(&n));
> inside emallc() to make your leak hunt easier.

Thanks for the feedback!

I have amended the patch (see attachement) and use setmalloctag()
and seteralloctag() to improve hunting for leaks as you suggest.
Also fixed a few typos in the slightly long commit message…

Will do some more testing tonight and commit tomorrow if all works
out fine.

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

From: Igor Böhm <igor@9lab.org>
Date: Wed, 10 Nov 2021 13:01:38 +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 (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.
---
diff a7ec6ee4e8103bb448f902e9b6d0dc69629abc57 f439fbb27f17b1fdc2b933bfb339a1ffa1887592
--- a/sys/src/cmd/rc/exec.c	Mon Nov  8 02:05:51 2021
+++ b/sys/src/cmd/rc/exec.c	Wed Nov 10 14:01:38 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();
 		}
--- a/sys/src/cmd/rc/subr.c	Mon Nov  8 02:05:51 2021
+++ b/sys/src/cmd/rc/subr.c	Wed Nov 10 14:01:38 2021
@@ -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;
 }
 

      reply	other threads:[~2021-11-11 10:08 UTC|newest]

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

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=783E98DBC7A1EBAD7AB87A2E5E0673D0@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).