Re: [9front] hjfs stack overflow

[Julius Schmidt <aiju@phicode.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2906 bytes --]

Thanks for finding this.

I've included a patch that uses an explicit stack.

Can somebody please test it and/or review it?

I've tried mischief's example and it seems to not crash but it seems mkdir 
or pwd truncates the path or something.

On Wed, 15 Aug 2018, Nick Owens wrote:

> there is a stack overflow in hjfs, trivially reproducible by executing:
>
> # set up a new temporary disk
> dd -if /dev/zero -of /tmp/hjfs.tmp -oseek 52428800 -count 1
> hjfs -n hjfs.tmp -f /tmp/hjfs.tmp -S -r -m 16
> echo newuser $user > /srv/hjfs.tmp.cmd
> mount -cC /srv/hjfs.tmp /n/hjfs.tmp
> cd /n/hjfs.tmp
>
> # crash hjfs
> mkdir -p `{seq 1000 | tr '\xa' /}
>
> the function willmodify is recursive, and appears to recurse for each
> element in the directory path. possible solutions i see are make it
> iterative, or have it call needstack() from libthread and return an error
> when out of stack.
>
> below is a stack trace with the majority of the recursive calls clipped out.
>
> /proc/143864/text:amd64 plan 9 executable
> /sys/lib/acid/port
> /sys/lib/acid/amd64
> $home/lib/acid
> acid: abort()+0x0 /sys/src/libc/9sys/abort.c:6
> needstack(n=0x80)+0xa5 /sys/src/libthread/sched.c:95
> x=0x217c2afefefefe
> _sched()+0x27 /sys/src/libthread/sched.c:109
> p=0x1424da0
> t=0x15b6bd8
> _threadrendezvous(tag=0x15b6df0,val=0x0)+0x132
> /sys/src/libthread/rendez.c:56
> l=0x40dc68
> t=0x15b6bd8
> ret=0x1424da0
> alt(alts=0x15b6e40)+0x1ed /sys/src/libthread/channel.c:172
> t=0x15b6bd8
> s=0x0
> n=0x0
> a=0x0
> c=0x0
> xa=0x15b6e40
> ca=0x0
> waiting=0x1
> allreadycl=0x100000000
> r=0xfefefefefefefefe
> runop(c=0x418500,v=0x15b6f28,nb=0xfefefefe00000000)+0x52
> /sys/src/libthread/channel.c:314
> a=0x418500
> recv(v=0x15b6f28)+0x28 /sys/src/libthread/channel.c:321
> getbuf(d=0x1435c10,off=0x4,nodata=0xfefefefe00000000,type=0xfefefefe00000004)+0x92
> /sys/src/cmd/hjfs/buf.c:268
> req=0x1435c10
> th=0x4184c0
> b=0xfefefefefefefefe
> chref(r=0x12c4,stat=0x0)+0x53 /sys/src/cmd/hjfs/fs1.c:18
> j=0xfefefefe000002c5
> rc=0x2c5fefefefe
> willmodify(l=0x1628098,fs=0x15fae18,nolock=0x1)+0x96
> /sys/src/cmd/hjfs/dump.c:133
> p=0x217f20
> d=0x1425780
> i=0xfefefefefefefefe
> r=0x20cea0
>
>
> [ ... ]
>
>
> willmodify(l=0x423640,fs=0x15fae18,nolock=0xffffffff00000001)+0x6f
> /sys/src/cmd/hjfs/dump.c:131
> p=0x1628d68
> d=0x15fae18
> i=0x21b8e7
> r=0x70020cea0
> willmodify(l=0x4236e0,fs=0x15fae18,nolock=0x0)+0x6f
> /sys/src/cmd/hjfs/dump.c:131
> p=0x1424da0
> d=0x20cea0
> i=0x203b88
> r=0x405101
> chancreat(ch=0x418020,name=0x15fbb84,perm=0x800001ff,mode=0xfefefefe00000000)+0xd9
> /sys/src/cmd/hjfs/fs2.c:119
> b=0x0
> l=0x0
> isdir=0x403f3800000001
> d=0x403f38
> f=0x20fba0
> workerproc()+0x1dd /sys/src/cmd/hjfs/9p.c:216
> ch=0x418020
> req=0x1628a98
> o=0x1628bc0
> launcheramd64(arg=0x0,f=0x2097b7)+0x10 /sys/src/libthread/amd64.c:11
> 0xfefefefefefefefe ?file?:0
> acid:
> echo kill > /proc/143864/ctl
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-diff; name=hjfs.patch, Size: 2232 bytes --]

diff -r f9a9f7e1c5e6 sys/src/cmd/hjfs/dump.c
--- a/sys/src/cmd/hjfs/dump.c	Wed Jul 11 16:05:03 2018 +0100
+++ b/sys/src/cmd/hjfs/dump.c	Thu Aug 16 13:00:08 2018 +0100
@@ -112,8 +112,8 @@
 	return -1;
 }
 
-int
-willmodify(Fs *fs, Loc *l, int nolock)
+static int
+willmodify1(Fs *fs, Loc *l)
 {
 	Buf *p;
 	Loc *m;
@@ -121,29 +121,20 @@
 	Dentry *d;
 	int rc;
 
-	if((l->flags & LDUMPED) != 0)
-		return 1;
-	if(!nolock){
-again:
-		runlock(fs);
-		wlock(fs);
-	}
-	if(l->next != nil && willmodify(fs, l->next, 1) < 0)
-		goto err;
 	rc = chref(fs, l->blk, 0);
 	if(rc < 0)
-		goto err;
+		return -1;
 	if(rc == 0){
 		dprint("willmodify: block %lld has refcount 0\n", l->blk);
 		werrstr("phase error -- willmodify");
-		goto err;
+		return -1;
 	}
 	if(rc == 1)
 		goto done;
 
 	p = getbuf(fs->d, l->next->blk, TDENTRY, 0);
 	if(p == nil)
-		goto err;
+		return -1;
 	d = getdent(l->next, p);
 	if(d != nil) for(i = 0; i < d->size; i++){
 		rc = getblk(fs, l->next, p, i, &r, GBREAD);
@@ -155,12 +146,12 @@
 phase:
 	werrstr("willmodify -- phase error");
 	putbuf(p);
-	goto err;
+	return -1;
 found:
 	rc = getblk(fs, l->next, p, i, &r, GBWRITE);
 	if(rc < 0){
 		putbuf(p);
-		goto err;
+		return -1;
 	}
 	if(rc == 0)
 		goto phase;
@@ -180,17 +171,50 @@
 	}
 done:
 	l->flags |= LDUMPED;
+	return 0;
+}
+
+int
+willmodify(Fs *fs, Loc *l, int nolock)
+{
+	Loc **st;
+	int sti, rc;
+	
+	if((l->flags & LDUMPED) != 0)
+		return 1;
+	if(!nolock){
+again:
+		runlock(fs);
+		wlock(fs);
+	}
+	st = emalloc(sizeof(Loc *));
+	*st = l;
+	sti = 0;
+	for(;;){
+		if((st[sti]->flags & LDUMPED) != 0 || st[sti]->next == nil)
+			break;
+		st = erealloc(st, (sti + 2) * sizeof(Loc *));
+		st[sti + 1] = st[sti]->next;
+		sti++;
+	}
+	rc = 0;
+	for(; sti >= 0; sti--){
+		rc = willmodify1(fs, st[sti]);
+		if(rc < 0){
+			free(st);
+			if(!nolock){
+				wunlock(fs);
+				rlock(fs);
+			}
+			return -1;
+		}
+	}
 	if(!nolock){
 		wunlock(fs);
 		rlock(fs);
 		if(chref(fs, l->blk, 0) != 1)
 			goto again;
 	}
-	return 0;
-err:
-	if(!nolock){
-		wunlock(fs);
-		rlock(fs);
-	}
-	return -1;
+	free(st);
+	return rc;
 }