hjfs stack overflow

hjfs stack overflow

[Nick Owens]

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

Re: [9front] hjfs stack overflow

[Julius Schmidt]

[-- 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;
 }

Re: [9front] hjfs stack overflow

[Nick Owens]

i tested the patch, and it is working here. hjfs no longer encounters
a stack overflow while creating very deep directories.

On Thu, Aug 16, 2018 at 5:03 AM, Julius Schmidt <aiju@phicode.de> wrote:
> 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
>
>
> from postmaster@ewsd:
> The following attachment had content that we can't
> prove to be harmless.  To avoid possible automatic
> execution, we changed the content headers.
> The original header was:
>
>         Content-Type: TEXT/x-diff; name=hjfs.patch
>         Content-Transfer-Encoding: BASE64
>         Content-ID: <alpine.LNX.2.00.1808161403200.15422@phi>
>         Content-Description:
>         Content-Disposition: attachment; filename=hjfs.patch