Re: [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process for qemu 9p proxy FS

[M. Mohan Kumar]

Stefan Hajnoczi wrote:
> On Tue, Nov 15, 2011 at 11:57 AM, M. Mohan Kumar<address@hidden>  wrote:
>    
> > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> > new file mode 100644
> > index 0000000..69daf7c
> > --- /dev/null
> > +++ b/fsdev/virtfs-proxy-helper.c
> > @@ -0,0 +1,271 @@
> > +/*
> > + * Helper for QEMU Proxy FS Driver
> > + * Copyright IBM, Corp. 2011
> > + *
> > + * Authors:
> > + * M. Mohan Kumar<address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2. See
> > + * the COPYING file in the top-level directory.
> > + */
> > +#include<stdio.h>
> > +#include<sys/socket.h>
> > +#include<string.h>
> > +#include<sys/un.h>
> > +#include<limits.h>
> > +#include<signal.h>
> > +#include<errno.h>
> > +#include<stdlib.h>
> > +#include<sys/resource.h>
> > +#include<sys/stat.h>
> > +#include<getopt.h>
> > +#include<unistd.h>
> > +#include<syslog.h>
> > +#include<sys/prctl.h>
> > +#include<sys/capability.h>
> > +#include<sys/fsuid.h>
> > +#include<stdarg.h>
> > +#include "bswap.h"
> >      
> Where is "bswap.h" used and why above<sys/socket.h>?
>    
I will fix it
>    
> > +#include<sys/socket.h>
> > +#include "qemu-common.h"
> > +#include "virtio-9p-marshal.h"
> > +#include "hw/9pfs/virtio-9p-proxy.h"
> > +
> > +#define PROGNAME "virtfs-proxy-helper"
> > +
> > +static struct option helper_opts[] = {
> > +    {"fd", required_argument, NULL, 'f'},
> > +    {"path", required_argument, NULL, 'p'},
> > +    {"nodaemon", no_argument, NULL, 'n'},
> > +};
> > +
> > +int is_daemon;
> >      
> static?
>
> Also, please use the bool type from<stdbool.h>, it makes it easier
> for readers who don't have to guess how the variable works (might be a
> bitfield or reference count too).
>
>    
I will fix it.
> > +static int socket_read(int sockfd, void *buff, ssize_t size)
> > +{
> > +    int retval;
> > +
> > +    do {
> > +        retval = read(sockfd, buff, size);
> > +    } while (retval<  0&&  errno == EINTR);
> > +    if (retval != size) {
> > +        return -EIO;
> > +    }
> >      
> Shouldn't this loop until size bytes have been read?
>
>    
Ok, I will fix this.
> > +    return retval;
> > +}
> > +
> > +static int socket_write(int sockfd, void *buff, ssize_t size)
> > +{
> > +    int retval;
> > +
> > +    do {
> > +        retval = write(sockfd, buff, size);
> > +    } while (retval<  0&&  errno == EINTR);
> > +    if (retval != size) {
> > +        return -EIO;
> >      
> We could pass the actual -errno here if retval<  0.
>
>    
Socket errors are treated fatal and the reason for failures are not used
by the code. When ever there is socket error, helper exits.


> > +    }
> > +    return retval;
> > +}
> > +
> > +static int read_request(int sockfd, struct iovec *iovec)
> > +{
> > +    int retval;
> > +    ProxyHeader header;
> > +
> > +    /* read the header */
> > +    retval = socket_read(sockfd, iovec->iov_base, sizeof(header));
> > +    if (retval != sizeof(header)) {
> > +        return -EIO;
> > +    }
> > +    /* unmarshal header */
> > +    proxy_unmarshal(iovec, 1, 0, "dd",&header.type,&header.size);
> > +    /* read the request */
> > +    retval = socket_read(sockfd, iovec->iov_base + sizeof(header), 
> > header.size);
> > +    if (retval != header.size) {
> > +        return -EIO;
> > +    }
> > +    return header.type;
> > +}
> >      
> Size checks are missing and we're trusting what the client sends!
>    
Could you please elaborate?
>    
> > +
> > +static void usage(char *prog)
> > +{
> > +    fprintf(stderr, "usage: %s\n"
> > +            " -p|--path<path>  9p path to export\n"
> > +            " {-f|--fd<socket-descriptor>} socket file descriptor to be used\n"
> > +            " [-n|--nodaemon] Run as a normal program\n",
> > +            basename(prog));
> > +}
> > +
> > +static int process_requests(int sock)
> > +{
> > +    int type;
> > +    struct iovec iovec;
> > +
> > +    iovec.iov_base = g_malloc(BUFF_SZ);
> > +    iovec.iov_len = BUFF_SZ;
> > +    while (1) {
> > +        type = read_request(sock,&iovec);
> > +        if (type<= 0) {
> > +            goto error;
> > +        }
> > +    }
> > +    (void)socket_write;
> > +error:
> > +    g_free(iovec.iov_base);
> > +    return -1;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    int sock;
> > +    char rpath[PATH_MAX];
> > +    struct stat stbuf;
> > +    int c, option_index;
> > +
> > +    is_daemon = 1;
> > +    rpath[0] = '\0';
> > +    sock = -1;
> > +    while (1) {
> > +        option_index = 0;
> > +        c = getopt_long(argc, argv, "p:nh?f:", helper_opts,
> > +&option_index);
> > +        if (c == -1) {
> > +            break;
> > +        }
> > +        switch (c) {
> > +        case 'p':
> > +            strcpy(rpath, optarg);
> >      
> Buffer overflow.  The whole thing would be simpler like this:
>
> const char *rpath = "";
> [...]
>      case 'p':
>          rpath = optarg;
>          break;
>
>    
I will fix it
> > +            break;
> > +        case 'n':
> > +            is_daemon = 0;
> > +            break;
> > +        case 'f':
> > +            sock = atoi(optarg);
> > +            break;
> > +        case '?':
> > +        case 'h':
> > +        default:
> > +            usage(argv[0]);
> > +            return -1;
> >      
> The convention is for programs to exit with 1 (EXIT_FAILURE) on error.
>
>    
I will fix it.
> > +            break;
> > +        }
> > +    }
> > +
> > +    /* Parameter validation */
> > +    if (sock == -1 || rpath[0] == '\0') {
> > +        fprintf(stderr, "socket descriptor or path not specified\n");
> > +        usage(argv[0]);
> > +        return -1;
> > +    }
> > +
> > +    if (lstat(rpath,&stbuf)<  0) {
> > +        fprintf(stderr, "invalid path \"%s\" specified?\n", rpath);
> >      
> sterror() would provide further details on what went wrong.
>    
Ok
>    
> > +        return -1;
> > +    }
> > +
> > +    if (!S_ISDIR(stbuf.st_mode)) {
> > +        fprintf(stderr, "specified path \"%s\" is not directory\n", rpath);
> > +        return -1;
> > +    }
> > +
> > +    if (is_daemon) {
> > +        if (daemon(0, 0)<  0) {
> > +            fprintf(stderr, "daemon call failed\n");
> > +            return -1;
> > +        }
> > +        openlog(PROGNAME, LOG_PID, LOG_DAEMON);
> > +    }
> > +
> > +    do_log(LOG_INFO, "Started");
> > +
> > +    if (chroot(rpath)<  0) {
> > +        do_perror("chroot");
> > +        goto error;
> > +    }
> > +    umask(0);
> > +
> > +    if (init_capabilities()<  0) {
> > +        goto error;
> > +    }
> > +
> > +    process_requests(sock);
> > +error:
> > +    do_log(LOG_INFO, "Done");
> > +    closelog();
> > +    return 0;
> > +}
> > diff --git a/hw/9pfs/virtio-9p-proxy.h b/hw/9pfs/virtio-9p-proxy.h
> > index f5e1a02..120e940 100644
> > --- a/hw/9pfs/virtio-9p-proxy.h
> > +++ b/hw/9pfs/virtio-9p-proxy.h
> > @@ -3,6 +3,16 @@
> >
> >   #define BUFF_SZ (4 * 1024)
> >
> > +#define proxy_unmarshal(in_sg, in_elem, offset, fmt, args...) \
> > +    v9fs_unmarshal(in_sg, in_elem, offset, 0 /* convert */, fmt, ##args)
> > +#define proxy_marshal(out_sg, out_elem, offset, fmt, args...) \
> > +    v9fs_marshal(out_sg, out_elem, offset, 0 /* convert */, fmt, ##args)
> > +
> > +union MsgControl {
> > +    struct cmsghdr cmsg;
> > +    char control[CMSG_SPACE(sizeof(int))];
> > +};
> >      
> This union isn't used in this patch.
>
>    
I will fix it.