From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RQbFD-0000sM-EI for qemu-devel@nongnu.org; Wed, 16 Nov 2011 03:53:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RQbFC-0002OO-1m for qemu-devel@nongnu.org; Wed, 16 Nov 2011 03:53:23 -0500 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:60640) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RQbF9-0002DC-Uv for qemu-devel@nongnu.org; Wed, 16 Nov 2011 03:53:21 -0500 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by e28smtp01.in.ibm.com (8.14.4/8.13.1) with ESMTP id pAG8rDPg020755 for ; Wed, 16 Nov 2011 14:23:13 +0530 Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pAG8pwI04677744 for ; Wed, 16 Nov 2011 14:21:58 +0530 Received: from d28av04.in.ibm.com (loopback [127.0.0.1]) by d28av04.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pAG8pwKI000416 for ; Wed, 16 Nov 2011 19:51:58 +1100 Message-ID: <4EC3797E.5090204@in.ibm.com> Date: Wed, 16 Nov 2011 14:21:10 +0530 From: "M. Mohan Kumar" MIME-Version: 1.0 References: <1321358265-10924-1-git-send-email-mohan@in.ibm.com> <1321358265-10924-4-git-send-email-mohan@in.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process for qemu 9p proxy FS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com Stefan Hajnoczi wrote: > On Tue, Nov 15, 2011 at 11:57 AM, M. Mohan Kumar 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 >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2. See >> + * the COPYING file in the top-level directory. >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include "bswap.h" >> > Where is "bswap.h" used and why above? > I will fix it > >> +#include >> +#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, 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 9p path to export\n" >> + " {-f|--fd} 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.