[9front] [PATCH] awk: don't write an extra NUL past the end of a block

[9front] [PATCH] awk: don't write an extra NUL past the end of a block

[Kristo]

When splitting a record into individual fields awk seems to write an
extra NUL at the end, which is not an issue when a record fits in the
default buffer. However, with larger records the buffer is allocated to
length of the string + 1, meaning that the extra NUL goes past the end
of the block and causes a "mem user overflow" panic.

diff 26c21f9b5d06296d13f40b53de14e22007e189c2 uncommitted
--- a/sys/src/cmd/awk/lib.c
+++ b/sys/src/cmd/awk/lib.c
@@ -287,7 +287,6 @@
 			while (*r != ' ' && *r != '\t' && *r != '\n' && *r != '\0');
 			*fr++ = 0;
 		}
-		*fr = 0;
 	} else if ((sep = *inputFS) == 0) {		/* new: FS="" => 1 char/field */
 		for (i = 0; *r != 0; r += w) {
 			char buf[UTFmax + 1];
@@ -320,7 +319,6 @@
 			if (*r++ == 0)
 				break;
 		}
-		*fr = 0;
 	}
 	if (i > nfields)
 		FATAL("record `%.30s...' has too many fields; can't happen", r);

Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block

[Jacob Moody]

Can you provide an example input that would generate this crash?

For awk things I usually tend to cross reference our bug fixes
against the onetrueawk to see if it was fixed there or not.
 From what I can tell onetrueawk also has this code:
https://github.com/onetrueawk/awk/blob/master/lib.c

I have a sneaking feeling that there are other cases
where this code is needed, but I'm not entirely sure.
It looks like there is some condition where we avoid that
loop, and that's why the code was there. Perhaps we
could just alloc length of string + 2?

Maybe give onetrueawk a try and see if you can reproduce the
crash there as well, and if not perhaps we should copy their fix.


Thanks,
moody

On 1/23/24 01:44, Kristo wrote:
> When splitting a record into individual fields awk seems to write an
> extra NUL at the end, which is not an issue when a record fits in the
> default buffer. However, with larger records the buffer is allocated to
> length of the string + 1, meaning that the extra NUL goes past the end
> of the block and causes a "mem user overflow" panic.
> 
> diff 26c21f9b5d06296d13f40b53de14e22007e189c2 uncommitted
> --- a/sys/src/cmd/awk/lib.c
> +++ b/sys/src/cmd/awk/lib.c
> @@ -287,7 +287,6 @@
>  			while (*r != ' ' && *r != '\t' && *r != '\n' && *r != '\0');
>  			*fr++ = 0;
>  		}
> -		*fr = 0;
>  	} else if ((sep = *inputFS) == 0) {		/* new: FS="" => 1 char/field */
>  		for (i = 0; *r != 0; r += w) {
>  			char buf[UTFmax + 1];
> @@ -320,7 +319,6 @@
>  			if (*r++ == 0)
>  				break;
>  		}
> -		*fr = 0;
>  	}
>  	if (i > nfields)
>  		FATAL("record `%.30s...' has too many fields; can't happen", r);
> 

Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block

[qwx]

On Tue Jan 23 17:53:35 +0100 2024, moody@posixcafe.org wrote:
> Can you provide an example input that would generate this crash?

Here's one:

; dd < /dev/zero -bs 4 -count 8k | tr '\x0' A | awk '{a=$1}'
8192+0 records in
8192+0 records out
mem user overflow
pool sbrkmem block 439db0
hdr 0a110c09 00008038 0020577c 00000000 41414141 41414141
tail 00000000 00000000 00000000 00000000 00000000 00000000 | ef2f00be 00008038
user data 41 41 41 41  41 41 41 00 | 00 f0 fa fe  00 00 00 00
panic: pool panic
sys: trap: fault read addr=0x0 pc=0x215070
awk 3897: suicide: sys: trap: fault read addr=0x0 pc=0x215070

; lstk
/proc/3897/text:amd64 plan 9 executable
acid: abort()+0x0 /sys/src/libc/9sys/abort.c:6
ppanic(fmt=0x40719b)+0x14b /sys/src/libc/port/malloc.c:166
	pv=0x417a18
	msg=0x420d90
	v=0x7fffffffea80
	n=0xffffea800000000a
panicblock(msg=0x4072ac,p=0x400c18,b=0x439db0)+0x52 /sys/src/libc/port/pool.c:718
blockcheck(b=0x439db0,p=0x400c18)+0x3f6 /sys/src/libc/port/pool.c:796
	t=0x441de0
	n=0x8038
	i=0x441de000000000
	q=0x441dc1
	a=0x439db0
	dsize=0x441dc500008009
	bq=0x441dc1
	eq=0x441dc5
arenamerge(p=0x400c18,bot=0x421a60,top=0x441df8)+0xf9 /sys/src/libc/port/pool.c:665
	newsize=0x215e7d000283f8
	btop=0x441e18
	bbot=0x439db0
poolnewarena(asize=0x8060,p=0x400c18)+0x1ec /sys/src/libc/port/pool.c:605
	a=0x441df8
	b=0x441e18
poolallocl(dsize=0x8009,p=0x400c18)+0x77 /sys/src/libc/port/pool.c:948
	bsize=0x21771e00008020
	ab=0x42f9f8
poolalloc(p=0x400c18,n=0x3ff0000000008009)+0x3c /sys/src/libc/port/pool.c:1190
	v=0x439dc0
malloc(size=0x8001)+0x20 /sys/src/libc/port/malloc.c:207
	v=0x208e3d
tostring(s=0x439dc0)+0x1d /sys/src/cmd/awk/tran.c:398
	p=0xa008
setsval(s=0x439dc0,vp=0x42f668)+0xee /sys/src/cmd/awk/tran.c:341
	fldno=0x417a1800000000
	t=0xe05c00417a18
assign(a=0x42f940,n=0xe038)+0xf5 /sys/src/cmd/awk/run.c:1141
	y=0x4263d0
	x=0x42f668
	yf=0x7fffffffee6b
	xf=0x8000
	v=0x8000
execute()+0xc4 /sys/src/cmd/awk/run.c:157
	a=0x42f928
	x=0x42f360
	nobj=0x20a78a0000e038
pastat(a=0x417fa0)+0x17 /sys/src/cmd/awk/run.c:1223
execute()+0xc4 /sys/src/cmd/awk/run.c:157
	a=0x42f968
	x=0xa000ffffee38
	nobj=0x2073300000e004
program(a=0x42f9c0)+0x14b /sys/src/cmd/awk/run.c:192
	x=0x2
execute()+0xc4 /sys/src/cmd/awk/run.c:157
	a=0x42f9a8
	x=0x40066e
	nobj=0x2070590000e003
run(a=0x42f9a8)+0x18 /sys/src/cmd/awk/run.c:131
main(argc=0x1,argv=0x7fffffffef98)+0x240 /sys/src/cmd/awk/main.c:169
	fs=0x0
	marg=0x0
_main+0x40 /sys/src/libc/amd64/main9.s:15


Doesn't happen with the patch.

Cheers,
qwx

Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block

[qwx]

On Tue Jan 23 17:53:35 +0100 2024, moody@posixcafe.org wrote:
> Maybe give onetrueawk a try and see if you can reproduce the
> crash there as well, and if not perhaps we should copy their fix.

Er, sorry;  this doesn't happen with bioawk on plan9, which is
based on onetrueawk iirc before it got littered with c99 stuff.

Cheers,
qwx

Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block

[Jacob Moody]

On my linux machine:
# nawk is onetrueawk
; dd </dev/zero bs=4 count=8k | tr '\0' A | nawk '{a=$1}'
8192+0 records in
8192+0 records out
32768 bytes (33 kB, 32 KiB) copied, 0.00933006 s, 3.5 MB/s

This works fine. No crash. I am not doubting this needs fixed,
I am doubtful of fixing it in this way being correct. It is still
not clear to me if there is some other input that is relying on
that extra line to be there. But it is clear that onetrueawk fixed
this using some other fix.

We should figure out how they solved it and/or prove that the cases
in which those loops never run are impossible to hit.

Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block

[Kristo]

On 23.1.2024 18.53, Jacob Moody wrote:
> Maybe give onetrueawk a try and see if you can reproduce the
> crash there as well, and if not perhaps we should copy their fix.

Not sure why I didn't realize to do this while debugging, indeed their
fix seems to be to allocate one more byte few lines above.

if ((fields = (char *) malloc(n+2)) == NULL) /* possibly 2 final \0s */

https://github.com/onetrueawk/awk/blob/master/lib.c#L394

I'm in favor of copying their fix as I can't say that I have a super
deep understanding of awk's internals.

Kristo

Aw: Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block

[Alexander Shendi]

Hi,


DISCLAIMER:
i haven't seen either the code of 9front's awk or onetrueawk's code.

However that doesn't keep me from speculating. I suspect the fields are stored as an array of NUL-terminated strings itself terminated by a NUL char. So you would need to allocate n+2 bytes for the last field.



> Gesendet: Dienstag, den 23.01.2024 um 18:50 Uhr
> Von: "Jacob Moody" <moody@posixcafe.org>
> An: 9front@9front.org
> Betreff: Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block
>
> On my linux machine:
> # nawk is onetrueawk
> ; dd </dev/zero bs=4 count=8k | tr '\0' A | nawk '{a=$1}'
> 8192+0 records in
> 8192+0 records out
> 32768 bytes (33 kB, 32 KiB) copied, 0.00933006 s, 3.5 MB/s
>
> This works fine. No crash. I am not doubting this needs fixed,
> I am doubtful of fixing it in this way being correct. It is still
> not clear to me if there is some other input that is relying on
> that extra line to be there. But it is clear that onetrueawk fixed
> this using some other fix.
>
> We should figure out how they solved it and/or prove that the cases
> in which those loops never run are impossible to hit.
>

Re: [9front] [PATCH] awk: don't write an extra NUL past the end of a block

[Jacob Moody]

Thanks! I committed the solution from onetrueawk.

On 1/23/24 12:32, Kristo wrote:
> On 23.1.2024 18.53, Jacob Moody wrote:
>> Maybe give onetrueawk a try and see if you can reproduce the
>> crash there as well, and if not perhaps we should copy their fix.
> 
> Not sure why I didn't realize to do this while debugging, indeed their
> fix seems to be to allocate one more byte few lines above.
> 
> if ((fields = (char *) malloc(n+2)) == NULL) /* possibly 2 final \0s */
> 
> https://github.com/onetrueawk/awk/blob/master/lib.c#L394
> 
> I'm in favor of copying their fix as I can't say that I have a super
> deep understanding of awk's internals.
> 
> Kristo