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 25514 invoked from network); 12 Apr 2021 14:05:40 -0000
Received: from 1ess.inri.net (216.126.196.35)
  by inbox.vuxu.org with ESMTPUTF8; 12 Apr 2021 14:05:40 -0000
Received: from mail.9lab.org ([168.119.8.41]) by 1ess; Mon Apr 12 08:49:52 -0400 2021
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9lab.org; s=20210803;
	t=1618231778;
	h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
	 to:to:cc:cc:mime-version:mime-version:  content-type:content-type:content-type:
	 content-transfer-encoding:content-transfer-encoding:content-transfer-encoding:
	 in-reply-to:in-reply-to; bh=2z8JzzzuQuGLV1mq28Jlfh1QDY+jOuPNl9adO3Y1QUQ=;
	b=TsRMzeR5XMY2ayeGgaT90WppkcnwaEIJ9+e0dekfg5BijwXH3RxLMOhn9T7QGuxA+WNwCd
	PF1WVzshaEI00WD8AShKwaeRqGQaDTjDsa/jjR7P1+97NAEOh1vtgABBKaFLfYzlYY/S+g
	vzFy1kVHRsxAWDmrjy0/IzSruzU6Or8=
Received: from term.localdomain (host-185-64-155-70.ecsnet.at [185.64.155.70])
	by mail.9lab.org (OpenSMTPD) with ESMTPSA id 8cd22b92 (TLSv1.2:ECDHE-RSA-AES256-SHA:256:NO);
	Mon, 12 Apr 2021 14:49:38 +0200 (CEST)
Message-ID: <6C9435B0EAB3ECBB65041EE29A4AA82D@9lab.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
To: 9front@9front.org
CC: igor@9lab.org, ori@eigenstate.org
Date: Mon, 12 Apr 2021 14:49:36 +0200
From: igor@9lab.org
In-Reply-To: <97FDE9A5306132622D1D4F91D0453FA9@eigenstate.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
List-ID: <9front.9front.org>
List-Help: <http://lists.9front.org>
X-Glyph: ➈
X-Bullshit: advanced ORM over SVG rails interface rails-scale metadata 
Subject: Re: [9front] upas/Mail: fix multiple suicides (use after free, double free) in mbox.c (patch)
Reply-To: 9front@9front.org
Precedence: bulk

Quoth ori@eigenstate.org:
> Quoth igor@9lab.org:
> > Below is a patch (inline) that fixes two variants of a suicide (use
> > after free, double free) in Mail/mbox.c on latest 9front 'Mit
> > Fruchtgeschmack' (explanation with reproducible test instructions
> > below) when messages are deleted/moved in bulk:
> > 
> > <patch>
> > diff -r 4dfbef4fa4ac sys/src/cmd/upas/Mail/mbox.c
> > --- a/sys/src/cmd/upas/Mail/mbox.c	Sat Apr 03 19:32:47 2021 +0200
> > +++ b/sys/src/cmd/upas/Mail/mbox.c	Fri Apr 09 02:54:10 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,13 @@
> >  			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.mesg[i] = nil;
> > +			mbox.nmesg--;
> > +		}else
> >  			mbox.mesg[j++] = m;
> >  	}
> 
> I think I prefer this patch:
>
> diff -r 503c5ef2d2b5 sys/src/cmd/upas/Mail/mbox.c
> --- a/sys/src/cmd/upas/Mail/mbox.c	Sun Apr 11 20:20:41 2021 +0200
> +++ b/sys/src/cmd/upas/Mail/mbox.c	Sun Apr 11 19:58:55 2021 -0700
> @@ -715,6 +715,7 @@
>  		else
>  			mbox.mesg[j++] = m;
>  	}
> +	mbox.nmesg = j;
>  		
>  	close(fd);
>  	fprint(mbox.ctl, "clean\n");
> 

I agree, this is simpler.

Can we set the slot of the deleted message to nil as well to avoid
keeping around pointers to already freed memory?

The following expresses this in a patch:

<snip>
diff -r 4dfbef4fa4ac sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Sat Apr 03 19:32:47 2021 +0200
+++ b/sys/src/cmd/upas/Mail/mbox.c	Mon Apr 12 14:35:54 2021 +0200
@@ -710,12 +710,14 @@
 
 	for(i = 0, j = 0; i < mbox.nmesg; i++){
 		m = mbox.mesg[i];
-		if((m->state & Szap) != 0)
+		if((m->state & Szap) != 0){
 			mesgfree(m);
-		else
+			mbox.mesg[i] = nil;
+		}else
 			mbox.mesg[j++] = m;
 	}
-		
+	mbox.nmesg = j;
+
 	close(fd);
 	fprint(mbox.ctl, "clean\n");
 }
</snip>

This is an optional request, if you disagree with it I am fine
with your proposal.