* [9front] cmd/rc: fix leaking runq->cmdfile (patch)
@ 2021-11-09 12:45 igor
2021-11-09 19:27 ` cinap_lenrek
0 siblings, 1 reply; 3+ messages in thread
From: igor @ 2021-11-09 12:45 UTC (permalink / raw)
To: 9front; +Cc: igor
[-- 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();
}
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [9front] cmd/rc: fix leaking runq->cmdfile (patch)
2021-11-09 12:45 [9front] cmd/rc: fix leaking runq->cmdfile (patch) igor
@ 2021-11-09 19:27 ` cinap_lenrek
2021-11-10 13:21 ` igor
0 siblings, 1 reply; 3+ messages in thread
From: cinap_lenrek @ 2021-11-09 19:27 UTC (permalink / raw)
To: 9front
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.
--
cinap
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [9front] cmd/rc: fix leaking runq->cmdfile (patch)
2021-11-09 19:27 ` cinap_lenrek
@ 2021-11-10 13:21 ` igor
0 siblings, 0 replies; 3+ messages in thread
From: igor @ 2021-11-10 13:21 UTC (permalink / raw)
To: 9front; +Cc: igor
[-- 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;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-11 10:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-09 12:45 [9front] cmd/rc: fix leaking runq->cmdfile (patch) igor
2021-11-09 19:27 ` cinap_lenrek
2021-11-10 13:21 ` igor
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).