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 6744 invoked from network); 2 Apr 2021 10:35:37 -0000 Received: from 1ess.inri.net (216.126.196.35) by inbox.vuxu.org with ESMTPUTF8; 2 Apr 2021 10:35:37 -0000 Received: from mail.9lab.org ([168.119.8.41]) by 1ess; Fri Apr 2 06:31:07 -0400 2021 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9lab.org; s=20210803; t=1617359453; 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:content-type: content-transfer-encoding:content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to; bh=bELP3+TAAS8FcG4eYIb/pWJV9bzhFnKMnubloWd4BdA=; b=pFvM97M2HACSLmdCnqdkCYkkXB7WS8Ec8WhFJBOz+vyEJsEwAQl26CzXGguTpWVUT9SYOg iMjpV/yt8hjEYD0Haw3bdpJ7Z6V2WiNH+KHyA8tChbfE7ZSrVj+9ztNGawMT8kfbLTXatN RvHO7Xf2cQPyI6Dod2cUU8X4KNOOx80= X-Spam: yes Received: from term.localdomain (host-185-64-155-70.ecsnet.at [185.64.155.70]) by mail.9lab.org (OpenSMTPD) with ESMTPSA id 17f95150 (TLSv1.2:ECDHE-RSA-AES256-SHA:256:NO); Fri, 2 Apr 2021 12:30:53 +0200 (CEST) Message-ID: <5B3CB75646F53123B72B31FF6925FDD7@9lab.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit To: 9front@9front.org CC: igor@9lab.org, cinap_lenrek@felloff.net Date: Fri, 02 Apr 2021 11:30:51 +0200 From: igor@9lab.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit List-ID: <9front.9front.org> List-Help: X-Glyph: ➈ X-Bullshit: encrypted property SOAP firewall replication method blockchain Subject: [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch) Reply-To: 9front@9front.org Precedence: bulk Quoth cinap_lenrek@felloff.net: [...] > The time to submit your unreleased bugfixes is now. :-) Here is a patch that fixes a (1) suicide and (2) memory leak in acme/ecmd.c (explanation with reproducible test instructions below): diff -r d9e940a768d1 sys/src/cmd/acme/ecmd.c --- a/sys/src/cmd/acme/ecmd.c Mon Oct 19 01:20:29 2020 +0200 +++ b/sys/src/cmd/acme/ecmd.c Fri Feb 05 14:18:06 2021 +0100 @@ -132,11 +132,11 @@ { File *f; - f = w->body.file; switch(editing){ case Inactive: return "permission denied"; case Inserting: + f = w->body.file; eloginsert(f, q, r, nr); return nil; case Collecting: @@ -157,11 +157,13 @@ if(nr == 0) return nil; r = skipbl(r, nr, &nr); - if(r[0] != '<') - return runestrdup(r); - /* use < command to collect text */ clearcollection(); - runpipe(t, '<', r+1, nr-1, Collecting); + if(r[0] != '<'){ + if((collection = runestrdup(r)) != nil) + ncollection += runestrlen(r); + }else + /* use < command to collect text */ + runpipe(t, '<', r+1, nr-1, Collecting); return collection; } To reproduce the suicide try running the following in acme: • 'Edit B cpu% broke echo kill>/proc/333310/ctl # acme cpu% acid 333310 /proc/333310/text:amd64 plan 9 executable /sys/lib/acid/port /sys/lib/acid/amd64 acid: lstk() edittext(nr=0x31,q=0x0,r=0x45aa10)+0x8 /sys/src/cmd/acme/ecmd.c:135 xfidwrite(x=0x461230)+0x28a /sys/src/cmd/acme/xfid.c:479 w=0x0 qid=0x5 fc=0x461390 t=0x1 nr=0x100000031 r=0x45aa10 eval=0x3100000000 a=0x405621 nb=0x500000001 err=0x419310 q0=0x100000000 tq0=0x80 tq1=0x8000000000 buf=0x41e8d800000000 xfidctl(arg=0x461230)+0x35 /sys/src/cmd/acme/xfid.c:52 x=0x461230 launcheramd64(arg=0x461230,f=0x22357e)+0x10 /sys/src/libthread/amd64.c:11 0xfefefefefefefefe ?file?:0 The suicide issue is caused by the following chain of events: • /sys/src/cmd/acme/ecmd.c:/^edittext is called at /sys/src/cmd/acme/xfid.c:479 passing nil as its first parameter: ... case QWeditout: r = fullrunewrite(x, &nr); if(w) err = edittext(w, w->wrselrange.q1, r, nr); else err = edittext(nil, 0, r, nr); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ... ...and /sys/src/cmd/acme/ecmd.c:/^edittext dereferences the first parameter that is *nil* at the first statement: char* edittext(Window *w, int q, Rune *r, int nr) { File *f; f = w->body.file; ^^^^^^^^^^^^^^^^^^^^^ This will crash if 'w' is *nil* switch(editing){ ... Moving the the derefernce of 'w' into the case where it is needed (see above patch) fixes the suicude. The memory leak is fixed in /sys/src/cmd/acme/ecmd.c:/^filelist. The current implementation of filelist(...) breaks its contract with its caller, thereby leading to a memory leak in /sys/src/cmd/acme/ecmd.c:/^B_cmd and /sys/src/cmd/acme/ecmd.c:/^D_cmd. The contract /sys/src/cmd/acme/ecmd.c:/^filelist seems to have with its callers is that in case of success it fills up a 'collection' that callers can then clear with a call to clearcollection(...). The fix above honours this contract and thereby removes the leak. After you apply the patch the following two tests should succeed: • Execute by select and middle click in a Tag: 'Edit B lib/profile' • Execute by select and middle click in a Tag: 'Edit B