kern/119079: [patch] DDB input routine reads/writes beyond end of buffer

[Michael Plass]

On Mar 6, 2008, at 2:01 AM, Robert Watson wrote:

>
> On Thu, 27 Dec 2007, Michael Plass wrote:
>
>>> Description:
>> 	The ddb input routine db_readline() includes the terminating newline
>> 	and NUL characters in the returned buffer, but it does not take this
>> 	into account when checking against the caller-supplied limit.
>>> How-To-Repeat:
>> 	Enter DDB and type enough characters to fill the buffer
>> 	(120 characters).  Hit enter, and then use the up-arrow key to
>> 	scroll back through history.  Note that it picks up garbage past the
>> 	end of the original line.
>>> Fix:
>> 	The patch checks the provided lsize and decreases by two to leave
>> 	room for the newline and NUL; it also clears these two characters,
>> 	because some of the code paths don't provide the terminating NUL.
>> 	(The patch also corrects a problem in history redraw when the cursor
>> 	is not at the end of the line while scrolling back though history.)
>
> Dear Michael,
>
> Thanks for this report; I'm able to reproduce both problems, and am  
> currently looking at your patch.  I'm going to immediately commit  
> the redraw fix, but did have one question about the remainder of  
> the patch below.
>
>> @@ -302,6 +302,10 @@
>> 	char *	lstart;
>> 	int	lsize;
>> {
>> +	if (lsize < 3)
>> +		return (0);
>> +	lstart[lsize - 1] = lstart[lsize - 2] = 0;
>> +	lsize -= 2;	/* allow space for newline and terminating NUL */
>
> You comment that you need to leave room for two bytes here --  
> terminating newline and nul.  Could you clarify in what cases the  
> NL may be missing?  It looks to me like the loop around db_inputchar 
> () effectively ensures that we never leave that loop without the  
> buffer containing an NL, and that the NL should fit in the buffer.   
> I concur on the nul termination reading.  So my question is: is it  
> not sufficient to simply do the following:
>
> 	lstart[lsize - 1] = 0;
> 	lsize--;
>
> Thanks,
>
> Robert N M Watson
> Computer Laboratory
> University of Cambridge
>
>> 	if (lsize != db_lhistlsize) {
>> 		/*
>> 		 * (Re)initialize input line history.  Throw away any
>> --- db_input_bufoverflow.patch ends here ---
>>
>>
>>> Release-Note:
>>> Audit-Trail:
>>> Unformatted:
>> _______________________________________________
>> freebsd-bugs at freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
>> To unsubscribe, send any mail to "freebsd-bugs- 
>> unsubscribe at freebsd.org"
>>

Robert,

I was able to reproduce the problem in a version rigged to run in  
userspace.
Check what happens when you just type enough to fill the buffer (or  
until it
beeps) and then hit return.  The normal chars are handled in the  
default case
of the big switch in db_inputchar(), and when the buffer is
full (db_le == db_lbuf_end), db_inputchar() returns 0.  The driving  
loop calls
again, this time with the newline, and that gets stored this way:
	    case '\n':
		/* FALLTHROUGH */
	    case '\r':
		*db_le++ = c;
		return (1);
leaving the newline in db_lbuf_end[0] and db_le == db_lbuf_end + 1,  
so that the terminating nul is stored at db_lbuf_end[1].

I've only lightly tested it, but upon reexamination I think better fix
is to leave lsize alone in do_readline() and initialize
    db_lbuf_end = lstart + lsize - 2;
this will assure that each of the lines in the history buffer have  
proper
termination and storing the nul in the initialization won't be needed.

The patch against 1.38 then looks like this:
--- db_input.c.orig	2008-03-06 22:09:04.000000000 -0800
+++ db_input.c	2008-03-06 22:25:24.000000000 -0800
@@ -302,6 +302,8 @@
  	char *	lstart;
  	int	lsize;
  {
+	if (lsize < 2)
+		return(0);
  	if (lsize != db_lhistlsize) {
  		/*
  		 * (Re)initialize input line history.  Throw away any
@@ -316,7 +318,7 @@
  	db_force_whitespace();	/* synch output position */

  	db_lbuf_start = lstart;
-	db_lbuf_end   = lstart + lsize;
+	db_lbuf_end   = lstart + lsize - 2;
  	db_lc = lstart;
  	db_le = lstart;


Thanks for the care you've taken in committing this.
- Michael