Memory errors (w/patch)

Memory errors (w/patch)

[Kristaps Dzonsons]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]

Hi,

Found by valgrind, again.  I think this one's fairly straightforward... 
Enclosed patch should speak for itself!

This can be triggered by the following:

----
.TH PLOCKSTAT 1 "July 2007" "1.0" ""
.SH NAME
plockstat \-
\fB
----

Ok?

Best,

Kristaps

[-- Attachment #2: term.diff --]
[-- Type: text/plain, Size: 516 bytes --]

Index: term.c
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/term.c,v
retrieving revision 1.227
diff -u -p -r1.227 term.c
--- term.c	10 Aug 2014 23:54:41 -0000	1.227
+++ term.c	18 Aug 2014 13:30:17 -0000
@@ -220,7 +220,7 @@ term_flushln(struct termp *p)
 				break;
 			if (' ' == p->buf[i]) {
 				j = i;
-				while (' ' == p->buf[i])
+				while (i < p->col && ' ' == p->buf[i])
 					i++;
 				dv = (i - j) * (*p->width)(p, ' ');
 				vbl += dv;

Re: Memory errors (w/patch)

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Mon, Aug 18, 2014 at 03:30:57PM +0200:

> Found by valgrind, again.  I think this one's fairly
> straightforward...

Nothing about term_flushln() is simple.  Srsly!

> Enclosed patch should speak for itself!

I spent quite some time staring at this and trying to figure out
how this might cause a buffer overrun, and could find no possible
explanation short of the length of the output buffer line being an
exact power of two times 1024 and an accidental blank character
right behind the buffer, whatever memory might be there.  But that
clearly isn't the case for your test file.

So i instrumented term_flushln() with printf()s and looked at
what actually happens at this point in the code:

>>>plockstat<<< 9 >>>- <<< 12 0 1024
>>>plockstat -<<< 11 >>><<< 12 0 1024
>>>July<<< 4 >>>2007<<< 9 32 1024

The first string between >>> <<< is buf[0] to buf[i-1],
the first number is i, so buf[i] is blank,
the second string buf[i+1] to buf[pos-1],
the second number p->pos, the third buf[p->pos],
and the final one p->maxcols.

Finally, i realized the problem is *not* a buffer overrun - that is
really extremely unlikely to happen, though in theory it can:
Even when there are, by chance, garbage trailing blanks in the
buffer after the data, that doesn't matter because the variables
vbl and vend that are being set to bogus values are never again
used in this case.  So only if the *whole rest* of the buffer is
blanks, including the memory cell after it, does this become an
overflow...  The problem is just read access to uninitialized memory.
In the second line, buf[12] is read even though it was never
initialized.

It can be triggered by any *output* line having an explicit
trailing blank.

So yes, your patch is indeed correct.

Please commit!
  Ingo


> This can be triggered by the following:
> 
> ----
> .TH PLOCKSTAT 1 "July 2007" "1.0" ""
> .SH NAME
> plockstat \-
> \fB
> ----
> 
> Ok?
> 
> Best,
> 
> Kristaps

> Index: term.c
> ===================================================================
> RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/term.c,v
> retrieving revision 1.227
> diff -u -p -r1.227 term.c
> --- term.c	10 Aug 2014 23:54:41 -0000	1.227
> +++ term.c	18 Aug 2014 13:30:17 -0000
> @@ -220,7 +220,7 @@ term_flushln(struct termp *p)
>  				break;
>  			if (' ' == p->buf[i]) {
>  				j = i;
> -				while (' ' == p->buf[i])
> +				while (i < p->col && ' ' == p->buf[i])
>  					i++;
>  				dv = (i - j) * (*p->width)(p, ' ');
>  				vbl += dv;
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv