[PATCH 2/4] isrv_identd: Fix off-by-one buffer overflow

[Denys Vlasenko]

On Thu, Jan 2, 2014 at 11:13 PM, Ryan Mallon <rmallon at gmail.com> wrote:
> The buf->buf buffer in do_rd() is explicitly NUL terminated. If the
> total number of bytes read is equal to the buffer size, then the NUL
> terminator will be written at the byte after the end of the buffer.
> Fix this by reducing the size of the read by 1 byte.

Thanks. Applied this part...

> Also removed the check for buf->pos being less than
> (int)sizeof(buf->buf). This is unnecessary since buf->pos cannot
> exceed sizeof(buf->buf), and was also incorrect since it was checked
> after buf->buf[buf->pos] was assigned with a NUL value. Removing it
> fixes a spurious warning from Smatch (http://smatch.sourceforge.net/).

> @@ -70,7 +71,7 @@ static int do_rd(int fd, void **paramp)
>         p = strpbrk(cur, "\r\n");
>         if (p)
>                 *p = '\0';
> -       if (!p && sz && buf->pos <= (int)sizeof(buf->buf))
> +       if (!p && sz)
>                 goto ok;
>         /* Terminate session. If we are in server mode, then
>          * fd is still in nonblocking mode - we never block here */

This check means "if we still have some space in the buffer
to read into, continue reading peer's data; if not, then terminate
(looks like peer is maliciously sending us too much data)"

You are right that the test is off-by-one, but removing it altogether
changes intended logic: now, we will wait for more data
even if buffer is full and we know we can't use more data.

Looks like such change will be ok: we don't expect "good"
clients to send such large requests.

Applying it too. Thanks!