[9front] cmd/upas/Mail: fix suicide (double free) in mbox.c (patch)

[igor@9lab.org]

Here is a patch that fixes two variants of a suicide in Mail/mbox.c on
latest 9front 'Mit Fruchtgeschmack' (explanation with reproducible
test instructions below):

<patch>
diff -r 87d8e72ffb5c sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Tue Mar 23 16:33:32 2021 -0700
+++ b/sys/src/cmd/upas/Mail/mbox.c	Sun Apr 04 22:03:25 2021 +0200
@@ -675,10 +675,11 @@
 static void
 mbflush(char **, int)
 {
-	int i, j, ln, fd;
+	int i, j, ln, fd, nmesg;
 	char *path;
 	Mesg *m, *p;
 
+	nmesg = mbox.nmesg;
 	path = estrjoin(maildir, "/ctl", nil);
 	fd = open(path, OWRITE);
 	free(path);
@@ -708,11 +709,12 @@
 			mbredraw(m->child[j], 1, 1);
  	}
 
-	for(i = 0, j = 0; i < mbox.nmesg; i++){
+	for(i = 0, j = 0; i < nmesg; i++){
 		m = mbox.mesg[i];
-		if((m->state & Szap) != 0)
+		if((m->state & Szap) != 0){
 			mesgfree(m);
-		else
+			mbox.nmesg--;
+		}else
 			mbox.mesg[j++] = m;
 	}
</patch>

To reproduce the suicide create a dummy IMAP folder, copy some emails
to it, and then select all emails and middle click 'Delmesg' followed by
middle clicking 'Put'.

Here is how to create a dummy IMAP folder 'Einhorn' and copy 50 mails
from your Inbox to it without leaving the comfort of 9front (replace
mail.9lab.org with your IMAP server):

<snip>
term% tlsclient tcp!mail.9lab.org!imaps
a  login YOURUSER YOURPASSWORD
b  create Einhorn
c  select Inbox
d  copy 1:50 Einhorn
e  logout
</snap>

Now you can open the Einhorn folder (replace mail.9lab.org with your
URL and igor with your user name):

<snip>
term% echo open /imaps/mail.9lab.org/igor/Einhorn Einhorn  >/mail/fs/ctl
</snap>

Finally, select and middle click 'Mail Einhorn' in acme to hopefully
reproduce the issue.

Now you can try to remove all mails from the Einhorn folder by
selecting all messages and middle clicking 'Delmesg' followed by
'Put'.

There is a very high chance upas/Mail will commit suicide like this:

<snip>
term% broke
echo kill>/proc/1136/ctl # Mail
term% acid 1136
/proc/1136/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: lstk()
strcmp(s2=0x463e88)+0x12 /sys/src/libc/port/strcmp.c:10
mesgmatch(m=0x412b00,name=0x463e88,digest=0x412c80)+0x24 /sys/src/cmd/upas/Mail/mesg.c:483
mesglookup(digest=0x412c80)+0x56 /sys/src/cmd/upas/Mail/mesg.c:496
	buf=0x2f33
	i=0x2f3300000001
delete(digest=0x412c80)+0x13 /sys/src/cmd/upas/Mail/mbox.c:336
changemesg(pm=0x4236b8)+0x181 /sys/src/cmd/upas/Mail/mbox.c:838
	digest=0x412c80
	action=0x412b00
	r=0x40fe00
mbmain(cmd=0x40fc80)+0x240 /sys/src/cmd/upas/Mail/mbox.c:966
	a=0x40fda0
	ev=0x40ee90
	psee=0x4236b8
	pshow=0xfefefefefefefefe
	psend=0xfefefefefefefefe
launcheramd64(arg=0x40fc80,f=0x2023d5)+0x10 /sys/src/libthread/amd64.c:11
</snap>

With the above commands to copy mails between folders you can create
more variations of deleting messages in bulk from the 'Einhorn' test
folder without breaking any sweat. Chances are high you will run into
another suicide like this one:

<snip>
panic: D2B called on non-block 40f9e8 (double-free?)
Mail 1967: suicide: sys: trap: fault read addr=0x0 pc=0x20fbdd
...
term% broke
echo kill>/proc/1967/ctl # Mail
term% acid 1967
/proc/1967/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: lstk()
abort()+0x0 /sys/src/libc/9sys/abort.c:6
ppanic(fmt=0x4055b1)+0x14b /sys/src/libc/port/malloc.c:166
	pv=0x407068
	msg=0x406f50
	v=0x46a810
	n=0x46a8100000002d
D2B(v=0x40f168)+0x6a /sys/src/libc/port/pool.c:926
	a=0x40f160
poolfreel(v=0x40f168,p=0x4001d8)+0x23 /sys/src/libc/port/pool.c:1153
	ab=0x407068
poolfree(p=0x4001d8,v=0x40f168)+0x3e /sys/src/libc/port/pool.c:1287
free()+0x26 /sys/src/libc/port/malloc.c:250
mesgclear(m=0x47ed80)+0x52 /sys/src/cmd/upas/Mail/mesg.c:26
	i=0x202ce000000001
mesgfree(m=0x47ed80)+0x19 /sys/src/cmd/upas/Mail/mesg.c:47
mbflush()+0x217 /sys/src/cmd/upas/Mail/mbox.c:714
	path=0x47fd00
	fd=0x10
	i=0x20231400000004
	p=0x0
	m=0x47ed80
	j=0x400000000
doevent(ev=0x494040)+0x11f /sys/src/cmd/upas/Mail/mbox.c:913
	f=0x494058
	nf=0x49405800000001
	p=0x406b98
mbmain(cmd=0x40fc80)+0x205 /sys/src/cmd/upas/Mail/mbox.c:962
	a=0x40fda0
	ev=0x494040
	psee=0x494e60
	pshow=0xfefefefefefefefe
	psend=0xfefefefefefefefe
launcheramd64(arg=0x40fc80,f=0x2023d5)+0x10 /sys/src/libthread/amd64.c:11
</snap>

So it looks like we are either operating on data that points to locations that
are no longer allocated, i.e. the symptoms point to a double free.

After reviewing the code paths outlined in the stack trace as well as
some surrounding logic, I think the problem is due to not decrementing
'mbox.nmesg' when messages are freed.  A quick grep in cmd/upas/Mail
confirms this:

<snip>
term% g nmesg
mail.h:134: 	int	nmesg;
mail.h:137: 	Mesg	*openmesg;
mesg.c:383: 	m->qnext = mbox.openmesg;
mesg.c:384: 	mbox.openmesg = m;
mesg.c:422: 	for(pm = &mbox.openmesg; *pm != nil; pm = &(*pm)->qnext)
mesg.c:495: 	for(i = 0; i < mbox.nmesg; i++)
mbox.c:154: 	for(i = 0; i < mbox.nmesg; i++){
mbox.c:167: 	assert(n + o <= mbox.nmesg);
mbox.c:199: 	for(i = 0; i < mbox.nmesg; i++)
mbox.c:259: 	if(mbox.nmesg == mbox.mesgsz){
mbox.c:266: 		idx = mbox.nmesg;
mbox.c:267: 	memmove(&mbox.mesg[idx + 1], &mbox.mesg[idx], (mbox.nmesg - idx)*sizeof(Mesg*));
mbox.c:269: 	mbox.nmesg++;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
mbox.c:274: 	if(mbox.hashsz <= 2*(mbox.nmesg + mbox.ndead)){
mbox.c:277: 		if(mbox.hashsz <= 2*mbox.nmesg)
mbox.c:439: 	qsort(mbox.mesg, mbox.nmesg, sizeof(Mesg*), cmpmesg);	
mbox.c:687: 	for(i = 0; i < mbox.nmesg; i++){
mbox.c:711: 	for(i = 0, j = 0; i < mbox.nmesg; i++){
mbox.c:749: 	for(i = 0; i < mbox.nmesg; i++){
mbox.c:771: 	for(m = mbox.openmesg; m != nil; m = m->qnext)
mbox.c:789: 	for(i = idx; i < mbox.nmesg; i++)
</snap>

We only increment 'mbox.nmesg' (see highlighted line), but there does
not seem to be a place where it is decremented.

Since 'mbox.nmesg' is not decremented after freeing/deleting emails,
any for loop like this one:

	'for(i = idx; i < mbox.nmesg; i++)'

executed after freeing/deleting emails, may access already freed data.

The fix proposed in the above patch decrements 'mbox.nmesg' when
deleting emails to avoid double free suicides.

I have been running with this fix for a while and have not observed a
suicide since. I delete/free mails in bulk a lot after copying them
from my IMAP Inbox folder to other IMAP folders. The way to move emails
in IMAP is to copy and then delete. To copy mails I use some hacked up
rc scripts that I middle click in acme to automate the process.

Danke für das neueste Release, ich mag Fruchtgeschmack ☺

Cheers,
Igor