Stefan Hajnoczi wrote:
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>?
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.
