[9front] drawterm: fix suicide caused by off-by-one write and out-of-bound read in libmemdraw/draw.c (patch)

[9front] drawterm: fix suicide caused by off-by-one write and out-of-bound read in libmemdraw/draw.c (patch)

[boehm.igor]

Drawterm is my key interface to 9front VMs.  Recently, I have been
experiencing random SEGFAULTs, mostly with more complex window
scenarious where I had many acme instances, netsurf, Nail, faces, etc.

This was getting really annoying since I was also running various ssh
tunnels from acme to compile and debug C++ code that I was editing in
acme on 9front. Recreating that setup after every crash was a nuisance
so I tried to figure out what the root cause was and even managed to
fix it. Drawterm now runs smoothly without hickups ☺

Further below is the full story on how to reproduce the issues with
some reasoning as to why things break and how to fix them so you
can review and hopefully commit these fixes in one way or another.

If you quickly want to test these changes they are also available in
the following repo:

 • git@github.com:1g0rb0hm/drawterm.git

Here is the pull request that contains the fix:

 • https://github.com/1g0rb0hm/drawterm/pull/3

Here is the diff generated with hg on 9front against:

 • https://code.9front.org/hg/drawterm

<diff>
cpu% hg diff .
diff -r 8fd69c7dfe88 libmemdraw/draw.c
--- a/libmemdraw/draw.c	Wed Dec 23 15:58:36 2020 +0100
+++ b/libmemdraw/draw.c	Thu Feb 04 21:55:51 2021 +0100
@@ -1476,7 +1476,7 @@
 {
 	Buffer b;
 	Memimage *img;
-	int dx, isgrey, convgrey, alphaonly, copyalpha, i, nb;
+	int dx, isgrey, convgrey, alphaonly, copyalpha, i, j, nb;
 	uchar *begin, *end, *r, *w, *rrepl, *grepl, *brepl, *arepl, *krepl;
 	uchar ured, ugrn, ublu;
 	ulong u;
@@ -1526,7 +1526,8 @@
 	krepl = replbit[img->nbits[CGrey]];
 
 	for(i=0; i<dx; i++){
-		u = r[0] | (r[1]<<8) | (r[2]<<16) | (r[3]<<24);
+		for(j = 0, u = 0 ; j < 4 && r+j < end ; j++)
+			u |= r[j] << (8*j);
 		if(copyalpha) {
 			*w++ = arepl[(u>>img->shift[CAlpha]) & img->mask[CAlpha]];
 		}
@@ -2136,8 +2137,8 @@
 						*dp ^= (v ^ *dp) & lm;
 						dp++;
 					}
-					memset(dp, v, dx);
-					dp += dx;
+					memset(dp, v, dx-1);
+					dp += dx-1;
 					*dp ^= (v ^ *dp) & rm;
 				}
 			}
</diff>


To reproduce the errors consistently with enough information to trace
down the root cuase, compile drawterm with the following options:

	CFDEBUG=-fno-omit-frame-pointer -fsanitize=address -O0
	CFLAGS=$(CFDEBUG) -Wall -Wno-gnu-designator -Wno-missing-braces -g  -I$(ROOT) -I$(ROOT)/include -I$(ROOT)/kern -c -D_THREAD_SAFE $(PTHREAD)

	LDDEBUG=-fsanitize=address
	LDADD=-g -framework AppKit -framework OpenGL $(LDDEBUG)

These options use ASAN in combination with clang/llvm and ensure we
get a proper stack trace pointing to the guilty statement.  Without
these options the stack trace you get is useless.

# 1. Issue: off-by-one write in libmemdraw/draw.c:/^memoptdraw

In libmemdraw/draw.c:/^memoptdraw an off by one write causes a
SEGFAULT.  The fault typically occurres in complex drawterm sessions
(i.e.  you have to stress the draw routines).  The best way to
reproduce it was to run two instances of netsurf, catclock, and doom.

Running a complex drawterm session (where drawterm was compiled with
above options) will then crash as follows:

Application Specific Information:
=================================================================
==81878==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61a0000b210c at pc 0x0001084224fb bp 0x70000d5065b0 sp 0x70000d5065a8
READ of size 1 at 0x61a0000b210c thread T20
    #0 0x1084224fa in memoptdraw draw.c:2141
    #1 0x10841ef62 in memimagedraw draw.c:158
    #2 0x1084697cc in memdraw draw.c:68
    #3 0x108290814 in drawmesg devdraw.c:1562
    #4 0x1082a418d in drawwrite devdraw.c:1319
    #5 0x1082fd0f2 in kwrite sysfile.c:474
    #6 0x1082fd377 in _syspwrite sysfile.c:499
    #7 0x1082ff7a4 in syspwrite sysfile.c:1059
    #8 0x10832223b in slavewrite exportsrv.c:638
    #9 0x108320fd0 in blockingslave exportsrv.c:495
    #10 0x108313602 in tramp posix.c:119
    #11 0x7fff682f8108 in _pthread_start+0x93 (libsystem_pthread.dylib:x86_64+0x6108)
    #12 0x7fff682f3b8a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1b8a)

SUMMARY: AddressSanitizer: heap-buffer-overflow draw.c:2141 in memoptdraw
Shadow bytes around the buggy address:
  0x1c34000163d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c34000163e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c34000163f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3400016400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3400016410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1c3400016420: 00[04]fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c3400016430: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c3400016440: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c3400016450: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c3400016460: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c3400016470: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

The following diff snippet shows the problematic code together with
its fix:

```c
@@ -2136,8 +2137,8 @@ memoptdraw(Memdrawparam *par)
 						*dp ^= (v ^ *dp) & lm;
 						dp++;
 					}
-					memset(dp, v, dx);
-					dp += dx;
+					memset(dp, v, dx-1);
+					dp += dx-1;
 					*dp ^= (v ^ *dp) & rm;
 				}
 			}
```

The actual SEFGAULT occurrs at the statement: 

• `*dp ^= (v ^ *dp) & rm;`

because in certain conditions it was writing out-of-bounds.  That was
identified by initially adding asserts(...) to narrow down the root
cause.

# 2.Issue: out-of-bound read in libmemdraw/draw.c:/^readbyte

In libmemdraw/draw.c:/^readbyte an out of bound read would read garbage
data and cause execution with ASAN to terminate with the following error:

Application Specific Information:
=================================================================
==83236==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d00003798c at pc 0x00010d4ab58c bp 0x70000ad2e840 sp 0x70000ad2e838
READ of size 1 at 0x61d00003798c thread T15
    #0 0x10d4ab58b in readbyte draw.c:1529
    #1 0x10d4a4f5e in greymaskread draw.c:1250
    #2 0x10d49c9e8 in alphadraw draw.c:720
    #3 0x10d491faa in memimagedraw draw.c:171
    #4 0x10d4dc7cc in memdraw draw.c:68
    #5 0x10d303814 in drawmesg devdraw.c:1562
    #6 0x10d31718d in drawwrite devdraw.c:1319
    #7 0x10d3700f2 in kwrite sysfile.c:474
    #8 0x10d370377 in _syspwrite sysfile.c:499
    #9 0x10d3727a4 in syspwrite sysfile.c:1059
    #10 0x10d39523b in slavewrite exportsrv.c:638
    #11 0x10d393fd0 in blockingslave exportsrv.c:495
    #12 0x10d386602 in tramp posix.c:119
    #13 0x7fff682f8108 in _pthread_start+0x93 (libsystem_pthread.dylib:x86_64+0x6108)
    #14 0x7fff682f3b8a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1b8a)
...
SUMMARY: AddressSanitizer: heap-buffer-overflow draw.c:1529 in readbyte
Shadow bytes around the buggy address:
  0x1c3a00006ee0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3a00006ef0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3a00006f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3a00006f10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3a00006f20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1c3a00006f30: 00[04]fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c3a00006f40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c3a00006f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3a00006f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3a00006f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c3a00006f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00


The problematic statement is:

• `u = r[0] | (r[1]<<8) | (r[2]<<16) | (r[3]<<24);`

The variable `r` is a uchar* pointer and we are, in certain
conditions, reading more memory than we are supposed to in the above
statement.

We have to be careful that at the index where we dereference `r` we
are not reading beyond the end of a buffer.  The fix below adds a
bounds check that never reads beyond those buffer bounds:


```c
@@ -1526,7 +1526,8 @@ readbyte(Param *p, uchar *buf, int y)
 	krepl = replbit[img->nbits[CGrey]];
 
 	for(i=0; i<dx; i++){
-		u = r[0] | (r[1]<<8) | (r[2]<<16) | (r[3]<<24);
+		for(j = 0, u = 0 ; j < 4 && r+j < end ; j++)
+		  u |= r[j] << (8*j);
 		if(copyalpha) {
 			*w++ = arepl[(u>>img->shift[CAlpha]) & img->mask[CAlpha]];
 		}
```

Sorry about the length of this mail.  Since this fixes bugs that have
existed for a long time and are tricky, I always like to add as much
context as possible to ease review.

Cheers,
Igor

Re: [9front] drawterm: fix suicide caused by off-by-one write and out-of-bound read in libmemdraw/draw.c (patch)

[ori]

Quoth boehm.igor@gmail.com:
>  	for(i=0; i<dx; i++){
> -		u = r[0] | (r[1]<<8) | (r[2]<<16) | (r[3]<<24);
> +		for(j = 0, u = 0 ; j < 4 && r+j < end ; j++)
> +			u |= r[j] << (8*j);
> 

This fix seems wrong: 'u = ...' is loading a pixel. Why
would we ever want to load *half* a pixel?

it seems like we either want to stop early, or ensure we
allocate the right size image.

Re: [9front] drawterm: fix suicide caused by off-by-one write and out-of-bound read in libmemdraw/draw.c (patch)

[boehm.igor]

Quoth ori@eigenstate.org:
> Quoth boehm.igor@gmail.com:
> >  	for(i=0; i<dx; i++){
> > -		u = r[0] | (r[1]<<8) | (r[2]<<16) | (r[3]<<24);
> > +		for(j = 0, u = 0 ; j < 4 && r+j < end ; j++)
> > +			u |= r[j] << (8*j);
> > 
> 
> This fix seems wrong: 'u = ...' is loading a pixel. Why
> would we ever want to load *half* a pixel?
> 
> it seems like we either want to stop early, or ensure we
> allocate the right size image.

Thanks for reviewing Ori. Yeah, you are right.

Let me go back and study the code a bit more and try to come
up with something that is not a hack.