|
From: | M. Mohan Kumar |
Subject: | Re: [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process for qemu 9p proxy FS |
Date: | Wed, 16 Nov 2011 14:21:10 +0530 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.19) Gecko/20110429 Fedora/2.0.14-1.fc15 SeaMonkey/2.0.14 |
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.
[Prev in Thread] | Current Thread | [Next in Thread] |