Re: [9front] upas/Mail: fix multiple suicides (use after free, double free) in mbox.c (patch)

[igor@9lab.org]

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.