From owner-freebsd-bugs@FreeBSD.ORG Fri Mar 7 06:57:22 2008 Return-Path: Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E81E01065670; Fri, 7 Mar 2008 06:57:21 +0000 (UTC) (envelope-from mfp49_freebsd@plass-family.net) Received: from plass-family.net (adsl-68-127-22-237.dsl.pltn13.pacbell.net [68.127.22.237]) by mx1.freebsd.org (Postfix) with ESMTP id A5ED48FC14; Fri, 7 Mar 2008 06:57:21 +0000 (UTC) (envelope-from mfp49_freebsd@plass-family.net) Received: from [192.168.0.192] (nat.plass-family.net [68.127.22.235]) by plass-family.net (Postfix) with ESMTP id 8B0582283A; Thu, 6 Mar 2008 22:45:35 -0800 (PST) In-Reply-To: <20080306095345.S12811@fledge.watson.org> References: <20071227224807.40C8F1702A@shuttle.plass-family.net> <20080306095345.S12811@fledge.watson.org> Mime-Version: 1.0 (Apple Message framework v753) Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed Message-Id: <6DC910A8-BA9A-4681-9D07-B87DE7261038@plass-family.net> Content-Transfer-Encoding: 7bit From: Michael Plass Date: Thu, 6 Mar 2008 22:38:56 -0800 To: Robert Watson X-Mailer: Apple Mail (2.753) Cc: freebsd-bugs@FreeBSD.org, Michael Plass , FreeBSD-gnats-submit@FreeBSD.org Subject: Re: kern/119079: [patch] DDB input routine reads/writes beyond end of buffer X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Mar 2008 06:57:22 -0000 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@freebsd.org mailing list >> http://lists.freebsd.org/mailman/listinfo/freebsd-bugs >> To unsubscribe, send any mail to "freebsd-bugs- >> unsubscribe@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