ref: afa5800b5b5782d71dd9347629b30bde0ac18635
parent: a398a097831f619fbebf064134b8931cb24ca051
author: cinap_lenrek <cinap_lenrek@felloff.net>
date: Fri Apr 2 11:51:15 EDT 2021
acme: fix suicide *and* resource leak in ecmd.c (thanks igor) To reproduce the suicide try running the following in acme: • 'Edit B <ls lib' by select and middle clicking in a window that is in your $home. There is a very high chance acme will commit suicide like this: <snip> 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 </snap> 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: <snip> ... case QWeditout: r = fullrunewrite(x, &nr); if(w) err = edittext(w, w->wrselrange.q1, r, nr); else err = edittext(nil, 0, r, nr); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ... </snap> ...and /sys/src/cmd/acme/ecmd.c:/^edittext dereferences the first parameter that is *nil* at the first statement: <snip> 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){ ... </snap> 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 <ls lib' The former lead to a resource leak that is now fixed. The latter lead to a suicide that is now fixed by moving the statement that dereferences the parameter to the location where it is needed, which is not the path used in the case of 'Edit B <ls'. Cheers, Igor
--- a/sys/src/cmd/acme/ecmd.c
+++ b/sys/src/cmd/acme/ecmd.c
@@ -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;
}