From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED autolearn=no autolearn_force=no version=3.4.4 Received: (qmail 13263 invoked from network); 9 Nov 2021 13:06:01 -0000 Received: from 4ess.inri.net (216.126.196.42) by inbox.vuxu.org with ESMTPUTF8; 9 Nov 2021 13:06:01 -0000 Received: from mail.9lab.org ([168.119.8.41]) by 4ess; Tue Nov 9 07:59:09 -0500 2021 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9lab.org; s=20210803; t=1636461947; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type; bh=AAWETNTOhI4gk0eeHvGUIXi/mswLQBG9dvKo4uEFpiA=; b=afBiqnNxw37ZBC+CgIEwNq5QInbXgvbs3tyAVT/r/w+f06CcV+D7rXd7m0ZtuSi0Yl5yh0 WDDsltsO6TKh4D32sUVA0eyLS4l+bdiessweH0g4zTFiA3x64vXo6U6WJ3jgrZlC5Qupta udOcb92Si8X1qiSfv5JBohjNc0te2AM= Received: from pjw (host-185-64-155-70.ecsnet.at [185.64.155.70]) by mail.9lab.org (OpenSMTPD) with ESMTPSA id 19e99a85 (TLSv1.2:ECDHE-RSA-CHACHA20-POLY1305:256:NO); Tue, 9 Nov 2021 13:45:47 +0100 (CET) Message-ID: <2203D6ED28C225EDCF76C0393E1960A8@9lab.org> To: 9front@9front.org CC: igor@9lab.org Date: Tue, 09 Nov 2021 13:45:45 +0100 From: igor@9lab.org MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="upas-fkqxswjujiqlsxlcrdvsmnkjni" List-ID: <9front.9front.org> List-Help: X-Glyph: ➈ X-Bullshit: shared self-healing firewall persistence SVG over SOAP lifecycle extension-oriented locator Subject: [9front] cmd/rc: fix leaking runq->cmdfile (patch) Reply-To: 9front@9front.org Precedence: bulk This is a multi-part message in MIME format. --upas-fkqxswjujiqlsxlcrdvsmnkjni Content-Disposition: inline Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit 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): From: Igor Böhm 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: 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: Another `runq->cmdfile` leak is present here (captured on a cpu server): 277 ├listen [tcp * /rc/bin/service ] 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 ] 320 │└listen [/net/tcp/1 tcp!*!567] 381 └closeproc cpu% leak -s 338 :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: 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` 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()); } 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: 277 ├listen [tcp * /rc/bin/service ] 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 ] 320 │└listen [/net/tcp/1 tcp!*!567] 381 └closeproc cpu% leak -s 338 :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 ...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(); } --upas-fkqxswjujiqlsxlcrdvsmnkjni Content-Disposition: attachment; filename=rc-fix-thread.cmdfile-leak.patch Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit From: Igor Böhm Date: Tue, 09 Nov 2021 12:33:08 +0000 Subject: [PATCH] rc: fix leaking runq->cmdfile To reproduce run the following on a terminal: 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: Another `runq->cmdfile` leak is present here (capture on a cpu server): 277 ├listen [tcp * /rc/bin/service ] 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 ] 320 │└listen [/net/tcp/1 tcp!*!567] 381 └closeproc cpu% leak -s 338 :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: 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` 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()); } 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: 277 ├listen [tcp * /rc/bin/service ] 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 ] 320 │└listen [/net/tcp/1 tcp!*!567] 381 └closeproc cpu% leak -s 338 :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 ...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(); } --upas-fkqxswjujiqlsxlcrdvsmnkjni--