Re: [Qemu-devel] [PATCH V4 04/13] hw/9pfs: File system helper process for qemu 9p proxy FS

["M. Mohan Kumar" <mohan@in.ibm.com>]

On Friday, December 09, 2011 12:01:14 AM Stefan Hajnoczi wrote:
> On Mon, Dec 05, 2011 at 09:48:41PM +0530, M. Mohan Kumar wrote:
> > +static int read_request(int sockfd, struct iovec *iovec, ProxyHeader
> > *header) +{
> > +    int retval;
> > +
> > +    /*
> > +     * read the request header.
> > +     */
> > +    iovec->iov_len = 0;
> > +    retval = socket_read(sockfd, iovec->iov_base, PROXY_HDR_SZ);
> > +    if (retval < 0) {
> > +        return retval;
> > +    }
> > +    iovec->iov_len = PROXY_HDR_SZ;
> > +    retval = proxy_unmarshal(iovec, 0, "dd", &header->type,
> > &header->size); +    if (retval < 0) {
> > +        return retval;
> > +    }
> > +    /*
> > +     * We can't process message.size > PROXY_MAX_IO_SZ, read the
> > complete +     * message from the socket and ignore it. This ensures
> > that +     * we can correctly handle the next request. We also return + 
> >    * ENOBUFS as error to indicate we ran out of buffer space. +     */
> > +    if (header->size > PROXY_MAX_IO_SZ) {
> > +        int count, size;
> > +        size = header->size;
> > +        while (size > 0) {
> > +            count = MIN(PROXY_MAX_IO_SZ, size);
> > +            count = socket_read(sockfd, iovec->iov_base + PROXY_HDR_SZ,
> > count); +            if (count < 0) {
> > +                return count;
> > +            }
> > +            size -= count;
> > +        }
> 
> I'm not sure recovery attempts are worthwhile here.  The client is
> buggy, perhaps just refuse further work.

But whats the issue in trying to recover in this case?
> 
> > +        return -ENOBUFS;
> > +    }
> 
> header->size is (signed) int and we didn't check for header->size < 0.
> Please use an unsigned type.

I will fix in next version

> 
> > +    if (chroot(rpath) < 0) {
> > +        do_perror("chroot");
> > +        goto error;
> > +    }
> > +    umask(0);
> 
> We haven't changed into the chroot yet, we need chdir("/").  Otherwise
> the current working directory is outside the chroot (and allows trivial
> escape).
I will fix in next version