Re: [PATCH v16 08/20] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers

[Stefan Hajnoczi]

On Mon, Jan 11, 2021 at 12:05:53AM -0500, Jagannathan Raman wrote:
> @@ -112,20 +132,36 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
>                  qio_channel_wait(ioc, G_IO_IN);
>              }
>              continue;
> -        } else if (len < 0) {
> -            goto cleanup;
> -        } else if (len == 0) {
> -            if (partial) {
> -                error_setg(errp,
> -                           "Unexpected end-of-file before all bytes were 
> read");
> -            } else {
> -                ret = 0;
> +        }
> +
> +        if (len <= 0) {
> +            size_t fd_idx = nfds ? *nfds : 0;

This loads uninitialized memory when len < 0 and the caller has not
initialized *nfds because qio_channel_readv_full() does not set *nfds =
0 in the failure case.

qio_channel_readv_full() should clear nfds at the start of the function:

  if (nfds) {
      *nfds = 0;
  }

> +            if (len == 0) {
> +                if (partial) {
> +                    error_setg(errp,
> +                               "Unexpected end-of-file before all bytes were 
> read");
> +                } else {
> +                    ret = 0;
> +                }
> +            }
> +
> +            while (fds && fd_idx) {
> +                close(*fds[fd_idx - 1]);

The type of fds is int **. Taking operator precedence into account, we
get:

  int *ptr = fds[fd_idx - 1]; /* fds = {&int1, &int2, &int3, ...} */
  close(*ptr);

That is not the intended behavior. I think this should be:

  close((*fds)[fd_idx - 1]);

> +                fd_idx--;
> +            }
> +
> +            if (fds) {
> +                g_free(*fds);
>              }
> +
>              goto cleanup;

Please clear fds and nfds so there is no way the caller can accidentally
use the freed values.

signature.asc
Description: PGP signature