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 10018 invoked from network); 11 Nov 2021 10:08:18 -0000 Received: from 4ess.inri.net (216.126.196.42) by inbox.vuxu.org with ESMTPUTF8; 11 Nov 2021 10:08:18 -0000 Received: from mail.9lab.org ([168.119.8.41]) by 4ess; Wed Nov 10 14:28:27 -0500 2021 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9lab.org; s=20210803; t=1636550516; 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: in-reply-to:in-reply-to; bh=19KlTYRmhr0nImuzyZ3erpwJh48DECAlGZxvmEc2qKE=; b=JcUpwBjZknARv6CYA5TuWhi4ROj0xIoV1U7VGx0XU9h3vjEs6+PFMvwtT4UAupAOwnSct6 mIQV8rnVd4R1X4dBRcimsgCSbXDqke6ksX5EObqQ63dQx1ooF81ztXfEdhGHCD4aX2f7iL uvwi//v6DQU3Mf15rG3NJjccQZNHb5Q= Received: from pjw (host-185-64-155-70.ecsnet.at [185.64.155.70]) by mail.9lab.org (OpenSMTPD) with ESMTPSA id 134439d7 (TLSv1.2:ECDHE-RSA-CHACHA20-POLY1305:256:NO); Wed, 10 Nov 2021 14:21:55 +0100 (CET) Message-ID: <783E98DBC7A1EBAD7AB87A2E5E0673D0@9lab.org> To: 9front@9front.org CC: igor@9lab.org Date: Wed, 10 Nov 2021 14:21:54 +0100 From: igor@9lab.org In-Reply-To: <316DC1762EF8EB5DC3E1756D964999CD@felloff.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="upas-tpqrhyeywobpiotzazjawukvhl" List-ID: <9front.9front.org> List-Help: X-Glyph: ➈ X-Bullshit: event XML shader-based session-based solution Subject: Re: [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-tpqrhyeywobpiotzazjawukvhl Content-Disposition: inline Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit 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. --upas-tpqrhyeywobpiotzazjawukvhl 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: Wed, 10 Nov 2021 13:01:38 +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 (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 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 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: 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 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 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; } --upas-tpqrhyeywobpiotzazjawukvhl--