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 28313 invoked from network); 2 Feb 2021 01:36:55 -0000 Received: from 1ess.inri.net (216.126.196.35) by inbox.vuxu.org with ESMTPUTF8; 2 Feb 2021 01:36:55 -0000 Received: from out0.migadu.com ([94.23.1.103]) by 1ess; Mon Feb 1 20:24:35 -0500 2021 Date: Tue, 2 Feb 2021 01:16:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pixelhero.dev; s=key1; t=1612228585; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2wQGLESIRselxpABoBD9220pfAUBkKvnk6BTGAPSkBs=; b=ohgL2OxVh86im6ZdzYz02nSVQUR083dTh8EenX+u9unoglEJIfy2rC3wRbI+FRwsncqsOV fI0m6HQRH60ga4wlfNfL5PW6jwYUqafvRQK0f76GK30IR7LkiKUvVkazPeLPZda0lkndxP VR5bPIF7Ee6UpUfxT52IcUve8lIXkofJjGxNa18p/52U6g2rYtEjXIJbV18BBK0SE+7CrF wxMmZ7wM8GkfJE7se5LsTHul9VrA5NeNwgL6W8/8t/5saN9Yqbbcn3vOtZw0xru9OmQoCG 2s3qnd3cNZJTv62ohlvBjbW4Ib7JXNg7aNNMr82RGS3ZzS/K0Eh33V9xMEI65A== X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Noam Preil To: 9front@9front.org Message-ID: In-Reply-To: <863CA88C234EA794D5D1804F1D51D318@gmail.com> References: <863CA88C234EA794D5D1804F1D51D318@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Correlation-ID: X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: noam@pixelhero.dev List-ID: <9front.9front.org> List-Help: X-Glyph: ➈ X-Bullshit: open anonymous hosting GPU callback browser Subject: Re: [9front] cmd/acme: fix resource leak in ecmd.c (patch) Reply-To: 9front@9front.org Precedence: bulk Uh, there is `r =3D list`. Feb 1, 2021 18:15:16 boehm.igor@gmail.com: > There is a resource leak in acme functions: > =E2=80=A2 /sys/src/cmd/acme/ecmd.c:/^B_cmd > =E2=80=A2 /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: > > =E2=80=A2 /sys/src/cmd/acme/ecmd.c:192 > > int > B_cmd(Text *t, Cmd *cp) > { > =C2=A0 Rune *list, *r, *s; > =C2=A0 int nr; > > =C2=A0 list =3D filelist(t, cp->text->r, cp->text->n); > =C2=A0 ^^^^^^^^^^^^^^^ > =C2=A0 if(list =3D=3D nil) > =C2=A0=C2=A0=C2=A0 editerror(Enoname); > =C2=A0 r =3D list; > =C2=A0 ... > =C2=A0 ... > =C2=A0 } > =C2=A0 clearcollection(); > =C2=A0 ^^^^^^^^^^^^^^^^^ > =C2=A0 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) > { > =C2=A0 if(nr =3D=3D 0) > =C2=A0=C2=A0=C2=A0 return nil; > =C2=A0 r =3D skipbl(r, nr, &nr); > =C2=A0 if(r[0] !=3D '<') > =C2=A0=C2=A0=C2=A0 return runestrdup(r); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > WHEN WE TAKE THE ABOVE PATH WE LEAK > WHAT runestrdup(r) RETURNS > =C2=A0 /* use < command to collect text */ > =C2=A0 clearcollection(); > =C2=A0 runpipe(t, '<', r+1, nr-1, Collecting); > =C2=A0 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.=C2=A0 The below simpl= y > checks if the pointer returned by filelist(...) is not equal to > 'collection'.=C2=A0 That is used as a sentinel to detect the case where > runestrdup(...) has been called.=C2=A0 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.=C2=A0 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 @@ > =C2=A0=C2=A0=C2=A0 if(nr > 0) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D skipbl(s+1, nr-1, &nr); > =C2=A0 } > + if(list !=3D collection) > +=C2=A0=C2=A0 free(list); > =C2=A0 clearcollection(); > =C2=A0 return TRUE; > } > @@ -278,6 +280,8 @@ > =C2=A0=C2=A0=C2=A0 if(nr > 0) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D skipbl(s+1, nr-1, &nr); > =C2=A0 }while(nr > 0); > + if(list !=3D collection) > +=C2=A0=C2=A0 free(list); > =C2=A0 clearcollection(); > =C2=A0 free(dir.r); > =C2=A0 return TRUE;