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_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FROM autolearn=no autolearn_force=no version=3.4.4 Received: (qmail 7121 invoked from network); 5 Feb 2021 11:12:50 -0000 Received: from 1ess.inri.net (216.126.196.35) by inbox.vuxu.org with ESMTPUTF8; 5 Feb 2021 11:12:50 -0000 Received: from mail-wm1-f49.google.com ([209.85.128.49]) by 1ess; Fri Feb 5 03:01:33 -0500 2021 Received: by mail-wm1-f49.google.com with SMTP id 190so5207712wmz.0 for <9front@9front.org>; Fri, 05 Feb 2021 00:01:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:message-id:to:cc:subject:date:in-reply-to:mime-version :content-transfer-encoding; bh=9S0H4kZ4L5pmBnehpyJ0DYH5XG+L0t3QKv/ZuFgmv3U=; b=gDMssn8y/JmEIpo+8AlOTCFdgY4mM4vnHP/hMvzk9EdQrxQVCTTM+CAghVkBXMS83W EnRDchPNPLKR/spIniolOX7/RNrgirOVsY6XNJB9X/PEmvqaq+mNLcDsf8Xm9Gsy+NHR CsajWvqfGrVB1NaXqO1xaK1U7Tu3IqrV8/aabpilPsVG3E+rYJMGuJYEhzCKJMrKyN+V RwD82fvPUnbjsqhyhs/9ponKzd8los72I3X32rd9rNuLDba884vbZgJ+o8k/m21jwF4m 9j8fVOK9PoxogIZVK2jYv4TfBqd4IAGYQ15x55IHnfTaw4NdS/Oq4mgTrfNxdb4NXx6g hJxA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:to:cc:subject:date:in-reply-to :mime-version:content-transfer-encoding; bh=9S0H4kZ4L5pmBnehpyJ0DYH5XG+L0t3QKv/ZuFgmv3U=; b=HvwPNOIRnITEAY89saqRL5D9Qb+pJPkkcVZmp/d/64NV2i0uGLBQQd8ECpOmqMshZz GKlKjitOgm3vKmpFY2PCBy7PV585sTXIQNKiy7pre6udJ27kOQzIvlocwXIY7pOunUcL 7JE6jR/5Gec3cTyMUIFrvuCx5y6qKaEkKV8jWP/4GQoeTDh12Pz+k4UHHxMcX6vLH08G re9NNFd7zh5pF4WZcuaLnqa6yzI+7l4gc9Nyu7yh+96qhD3hizX56vBEOw9K80dPCp4e l1VP12Z7bueRFnHnVX0QInd2CuTWo5WTYDZUI25qUA68EHcca9CETXf7S56yImVKsCV5 e7sg== X-Gm-Message-State: AOAM5329KPSxVx3vijSrlyJFlwrUs+POFuGrT0VKS2UqYb+aNDxfAOYB R6mmyvDTZOwZRKdE6vdGnM+Gttpod7E= X-Google-Smtp-Source: ABdhPJw4ZJP/ucT+6leM13i2+jlbh7LUobWKfHckkY1iNuOtxN66vnOYOSrcqerW5YsV/p2l/1LAbA== X-Received: by 2002:a05:600c:1986:: with SMTP id t6mr2387040wmq.93.1612511718602; Thu, 04 Feb 2021 23:55:18 -0800 (PST) Return-Path: Received: from term.home ([185.64.155.70]) by smtp.gmail.com with ESMTPSA id l5sm11209824wrv.44.2021.02.04.23.55.18 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Thu, 04 Feb 2021 23:55:18 -0800 (PST) From: boehm.igor@gmail.com X-Google-Original-From: igor@gmail.com Message-ID: To: 9front@9front.org CC: boehm.igor@gmail.com Date: Fri, 05 Feb 2021 08:55:16 +0100 In-Reply-To: <863CA88C234EA794D5D1804F1D51D318@gmail.com> 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: shader metadata hardware Subject: [9front] Re: cmd/acme: fix resource leak in ecmd.c (patch) Reply-To: 9front@9front.org Precedence: bulk Sorry, I have not provided a test case for when the leak happens. To trigger the leak type the following into an acme Tag (assuming sam.save exists): • Edit B sam.save and execute it (i.e. select and middle click). This is the case where memory is leaked. If the input to the B command comes from something preceded with '<' there is no leak. So for this case: • Edit B There is a resource leak in acme functions: > • /sys/src/cmd/acme/ecmd.c:/^B_cmd > • /sys/src/cmd/acme/ecmd.c:/^D_cmd > > I will describe the case for function B_cmd, the issue is the same for > D_cmd. > > In short, there can be a case where the heap allocated storage > returned by the /sys/src/cmd/acme/ecmd.c:/^filelist(...) function is > not freed. You can use the following as a starting point: > > • /sys/src/cmd/acme/ecmd.c:192 > > int > B_cmd(Text *t, Cmd *cp) > { > Rune *list, *r, *s; > int nr; > > list = filelist(t, cp->text->r, cp->text->n); > ^^^^^^^^^^^^^^^ > if(list == nil) > editerror(Enoname); > r = list; > ... > ... > } > clearcollection(); > ^^^^^^^^^^^^^^^^^ > return TRUE; > } > > Notice the two lines that are highlighted, namely the call to > filelist(...) and clearcollection(...). > > Now if we look at the impl of filelist(...) we have: > > /* string is known to be NUL-terminated */ > Rune* > filelist(Text *t, Rune *r, int nr) > { > if(nr == 0) > return nil; > r = skipbl(r, nr, &nr); > if(r[0] != '<') > return runestrdup(r); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > WHEN WE TAKE THE ABOVE PATH WE LEAK > WHAT runestrdup(r) RETURNS > /* use < command to collect text */ > clearcollection(); > runpipe(t, '<', r+1, nr-1, Collecting); > return collection; > ^^^^^^^^^^^^^^^^^^^^^^ > TAKING THIS PATH IS FINE AS clearcollection(...) > WILL FREE ANY HEAP ALLOCATED MEMORY > } > > So for the case where we *do not* return the 'collection' pointer we > leak memory that is allocated by 'runestrdup(...)'. > > There are probably more elegant solutions for this. The below simply > checks if the pointer returned by filelist(...) is not equal to > 'collection'. That is used as a sentinel to detect the case where > runestrdup(...) has been called. That is the case where the memory > returned by filelist(...) is *not* freed by clearcollection(...) and > requires an extra free. > > Somehow the explanation is much more complicated than I initially > intended. Hope you can still follow the reasoning and I haven't > screwed up somewhere. > > Here is the patch: > > diff -r 0b8c8ef6a3d4 sys/src/cmd/acme/ecmd.c > --- a/sys/src/cmd/acme/ecmd.c Tue Jan 19 15:18:57 2021 -0800 > +++ b/sys/src/cmd/acme/ecmd.c Mon Feb 01 22:24:56 2021 +0100 > @@ -204,6 +204,8 @@ > if(nr > 0) > r = skipbl(s+1, nr-1, &nr); > } > + if(list != collection) > + free(list); > clearcollection(); > return TRUE; > } > @@ -278,6 +280,8 @@ > if(nr > 0) > r = skipbl(s+1, nr-1, &nr); > }while(nr > 0); > + if(list != collection) > + free(list); > clearcollection(); > free(dir.r); > return TRUE; >