[PATCH RFA] libgloss read.c ... totally broken!

[J. Johnston]

cgd@broadcom.com wrote:
> 
> So, I was truly shocked to discover... libgloss/read.c is totally
> broken.  it seems to have been _always_ been broken in the public CVS,
> at least w.r.t. common sense and the read() semantics expected by the
> newlib stdio implementation.
> 
> I'm kinda surprised that nobody's noticed this problem before, I guess
> most people use libgloss w/ targets that don't use read.c, or just
> don't use input at all!
> 
> Two big problems, which this patch fixes:
> 
> (1) can write outside the buffer provided by the caller!
> 
>     the caller-provided buffer goes from buf[0] .. buf[nbytes-1].
> 
>     consider the 'for' loop when i == (nbytes-1), and the read
>     character is \n or \r:
> 
>         buf[nbytes-1] = the read character;
>         buf[nbytes-1+1] = \0;
> 
>     the latter is past the end of the user-supplied buffer!  (Also,
>     it's pointless to NUL-terminate the buffer.  8-)
> 
> (2) on empty input line, read returns 0!
> 
>     if first character returned by 'inbyte' is \n or \r.  the break
>     inside the loop is hit, but 'i' is never incremented.  0 is
>     returned.
> 
>     the stdio code (refill.c, ca. line 103) assumes that read
>     returning 0 means EOF, because, well, that's what normal UNIX
>     blocking read returning 0 means.  The Wrong Thing results.
> 
> The solution is:
> 
> * don't NUL-terminate the buffer,
> 
> * when you get CR or NL, make sure it's reflected in the return value
>   (by incrementing 'i' before the break).
> 
> So, more generally about libgloss read(), is there _any_ point in
> having it do this feeble attempt at line buffering?  Why not just
> return one character at a time?  (for apps which read lines from
> stdio, it'll work just fine...)
> 
> Then, once the existing buffering is squished: that would be a 'raw'
> read function, it might also be nice to have a 'cooked' read function
> that let people backspace, etc. while entering the line.  (The UNIX
> way to select between them is, of course, ioctl, but I'd be quite
> happy to do it for libgloss w/ a special function call that the user
> program could make if they wanted the cooked behaviour...)
> 
> (yes, that's right, somehow i've found people who want to use
> newlib+libgloss with simple, interactive programs on real raw
> hardware...  8-)
> 

Good catch Chris.  It is quite possible that something out there originally
depended on the old behavior, but I couldn't tell you what that would be.
The file was originally checked-in in 1995.  A number of the newer platforms
are supported by simulators and use a read syscall interface which is
handled by the simulator.

Patch checked in.

-- Jeff J.

> chris
> ===================================================================
> 2002-07-17  Chris Demetriou  <cgd@broadcom.com>
> 
>         * read.c (read): Don't assign past end of buffer, fix return value.
> 
> Index: read.c
> ===================================================================
> RCS file: /cvs/src/src/libgloss/read.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 read.c
> --- read.c      17 Mar 2000 22:48:49 -0000      1.1
> +++ read.c      18 Jul 2002 05:58:27 -0000
> @@ -31,7 +31,7 @@ _DEFUN (read, (fd, buf, nbytes),
>    for (i = 0; i < nbytes; i++) {
>      *(buf + i) = inbyte();
>      if ((*(buf + i) == '\n') || (*(buf + i) == '\r')) {
> -      (*(buf + i + 1)) = 0;
> +      i++;
>        break;
>      }
>    }