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

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

Stefan Hajnoczi wrote:
> On Tue, Nov 15, 2011 at 11:57 AM, M. Mohan Kumar<mohan@in.ibm.com>  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<mohan@in.ibm.com>
>> + *
>> + * 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.