9front - general discussion about 9front
 help / color / mirror / Atom feed
* [9front] call for patches for upcoming 9front release
@ 2021-04-01 17:02 cinap_lenrek
  2021-04-02  9:30 ` [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch) igor
  2021-04-02 14:46 ` [9front] call for patches for upcoming 9front release fulton
  0 siblings, 2 replies; 6+ messages in thread
From: cinap_lenrek @ 2021-04-01 17:02 UTC (permalink / raw)
  To: 9front

I want to finish a release this weekend.

There are the changes we have so far:

http://felloff.net/usr/cinap_lenrek/9front-83xx.changes.txt

The time to submit your unreleased bugfixes is now. :-)

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch)
  2021-04-01 17:02 [9front] call for patches for upcoming 9front release cinap_lenrek
@ 2021-04-02  9:30 ` igor
  2021-04-02 13:52   ` cinap_lenrek
  2021-04-02 14:46 ` [9front] call for patches for upcoming 9front release fulton
  1 sibling, 1 reply; 6+ messages in thread
From: igor @ 2021-04-02  9:30 UTC (permalink / raw)
  To: 9front; +Cc: igor, cinap_lenrek

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):

<patch>
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;
 }
</patch>

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch)
  2021-04-02  9:30 ` [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch) igor
@ 2021-04-02 13:52   ` cinap_lenrek
  0 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2021-04-02 13:52 UTC (permalink / raw)
  To: 9front

excellent explaination!

applied.

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] call for patches for upcoming 9front release
  2021-04-01 17:02 [9front] call for patches for upcoming 9front release cinap_lenrek
  2021-04-02  9:30 ` [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch) igor
@ 2021-04-02 14:46 ` fulton
  2021-04-02 15:31   ` cinap_lenrek
  1 sibling, 1 reply; 6+ messages in thread
From: fulton @ 2021-04-02 14:46 UTC (permalink / raw)
  To: 9front

Quoth cinap_lenrek@felloff.net:
> I want to finish a release this weekend.
> 
> There are the changes we have so far:
> 
> http://felloff.net/usr/cinap_lenrek/9front-83xx.changes.txt
> 
> The time to submit your unreleased bugfixes is now. :-)
> 
> --
> cinap
> 
Weather(1) patch http://okturing.com/src/10783/body

and whois(1) patch (thanks be to kvik for the rc scripting tip) http://okturing.com/src/10785/body

--
Fulton fulton.software!fulton

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] call for patches for upcoming 9front release
  2021-04-02 14:46 ` [9front] call for patches for upcoming 9front release fulton
@ 2021-04-02 15:31   ` cinap_lenrek
  0 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2021-04-02 15:31 UTC (permalink / raw)
  To: 9front

Applied the whois patch.

I have no idea what this weather thing is or if it is a good
idea to replace it with something that has completely different
arguments.

If it is broken, i'd rather just delete it instead of adding
more of these scripts that rely on some external webshit that
that we have no idea what they'r doing or if they will continue
to provide this service in the future.

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch)
@ 2021-02-05 14:23 boehm.igor
  0 siblings, 0 replies; 6+ messages in thread
From: boehm.igor @ 2021-02-05 14:23 UTC (permalink / raw)
  To: 9front; +Cc: boehm.igor

Here is a patch (second attempt) that fixes a (1) suicide and (2)
memory leak in acme/ecmd.c (full explanation with reproducible test
instructions below):

<patch>
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;
 }
</patch>

To reproduce the suicide issue 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 merrily 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 BLOWS UP IF w IS nil
	switch(editing){
...
</snap>

Moving the the derefernce of 'w' into the case where it is actually
needed (see above patch) fixes the suicude bug.

The memory leak is fixed properly in this second patch attempt, namely
in /sys/src/cmd/acme/ecmd.c:/^filelist.  The current implementation of
filelist(...) breaks the 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'
variable that callers can then clear with a call to
clearcollection(...).

The fix above honours this contract and thereby removes the leak
without requiring the weird check and free in B_cmd and D_cmd I had
earlier.
 
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'.

I hope that with this:
• patch
• bug description
• instruction on how to reproduce
• and explanation of the fixes

things are more likely to be accepted.

Let me know if you have any further questions and comments.

Cheers,
Igor


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-04-03  8:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 17:02 [9front] call for patches for upcoming 9front release cinap_lenrek
2021-04-02  9:30 ` [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch) igor
2021-04-02 13:52   ` cinap_lenrek
2021-04-02 14:46 ` [9front] call for patches for upcoming 9front release fulton
2021-04-02 15:31   ` cinap_lenrek
  -- strict thread matches above, loose matches on Subject: below --
2021-02-05 14:23 [9front] cmd/acme: fix suicide *and* resource leak in ecmd.c (patch) boehm.igor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).