On Tue, Nov 15, 2011 at 11:57 AM, M. Mohan Kumar <mo...@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 <mo...@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>? > +#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). > +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? > + 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. > + } > + 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! > + > +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; > + 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. > + 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. > + 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. Stefan