On Wed, 12 Sep 2018 12:04:21 +0800, Michael Mikonos <[email protected]> wrote:
> On Tue, Sep 11, 2018 at 11:25:52AM -0700, Ori Bernstein wrote: > > On Tue, 11 Sep 2018 15:36:49 +0800 > > Michael Mikonos <[email protected]> wrote: > > > > > Hello, > > > > > > Sometimes vmd doesn't seem to check the result of malloc/calloc. > > > I tried to preserve the existing behavour w.r.t. return values > > > for the functions modified; some functions returned 1 on error > > > while others return -1. Does this look correct? > > > > > > - Michael > > > > > > > > > > > Index: vioqcow2.c > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/vmd/vioqcow2.c,v > > > retrieving revision 1.2 > > > diff -u -p -u -r1.2 vioqcow2.c > > > --- vioqcow2.c 11 Sep 2018 04:06:32 -0000 1.2 > > > +++ vioqcow2.c 11 Sep 2018 07:29:10 -0000 > > > @@ -202,6 +202,9 @@ qc2_open(struct qcdisk *disk, int fd) > > > } > > > > > > disk->l1 = calloc(disk->l1sz, sizeof *disk->l1); > > > + if (disk->l1 == NULL) > > > + return -1; > > > + > > > if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off) > > > != 8*disk->l1sz) { > > > free(disk->l1); > > > @@ -237,6 +240,8 @@ qc2_open(struct qcdisk *disk, int fd) > > > basepath[backingsz] = 0; > > > > > > disk->base = calloc(1, sizeof(struct qcdisk)); > > > + if (disk->base == NULL) > > > + return -1; > > > > This early return leaks disk->l1. The other vioqcow2/vioraw changes > > look fine to me. > > Thanks for the feedback. > The other eary returns which currently free disk->base don't free > disk->l1. I can change those error cases, and this calloc() one, to free > disk->l1 if that's what is intended. > This is a bit more involved, but I think better: I moved the init of the various fields in disk up before any reads are done, and initialized l1, so that qc2_close will work at any point (at worst, it's a no-op). I added a virtio_shutdown function that acutally calls the close function -- it shouldn't matter now, since we basically sync on every operation, but I think it'll be important once we start trying to improve speed. And then, I made all the cleanup use it. Thanks for finding the issues. diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c index 7c7e4857695..ba871006b70 100644 --- usr.sbin/vmd/vioqcow2.c +++ usr.sbin/vmd/vioqcow2.c @@ -77,7 +77,6 @@ struct qcdisk { int fd; uint64_t *l1; - char *scratch; off_t end; uint32_t clustersz; off_t disksz; /* In bytes */ @@ -161,17 +160,19 @@ qc2_open(struct qcdisk *disk, int fd) size_t i; int version; + pthread_rwlock_init(&disk->lock, NULL); + disk->fd = fd; + disk->base = NULL; + disk->l1 = NULL; + if (pread(fd, &header, sizeof header, 0) != sizeof header) { log_warn("%s: short read on header", __func__); - return -1; + goto error; } if (strncmp(header.magic, "QFI\xfb", 4) != 0) { log_warn("%s: invalid magic numbers", __func__); - return -1; + goto error; } - pthread_rwlock_init(&disk->lock, NULL); - disk->fd = fd; - disk->base = NULL; disk->clustersz = (1ull << be32toh(header.clustershift)); disk->disksz = be64toh(header.disksz); @@ -182,6 +183,7 @@ qc2_open(struct qcdisk *disk, int fd) disk->refoff = be64toh(header.refoff); disk->nsnap = be32toh(header.snapcount); disk->snapoff = be64toh(header.snapsz); + /* * The additional features here are defined as 0 in the v2 format, * so as long as we clear the buffer before parsing, we don't need @@ -196,23 +198,25 @@ qc2_open(struct qcdisk *disk, int fd) * We only know about the dirty or corrupt bits here. */ if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) { - log_warn("%s: unsupported features %llx", __func__, + log_warnx("%s: unsupported features %llx", __func__, disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)); - return -1; + goto error; } disk->l1 = calloc(disk->l1sz, sizeof *disk->l1); - if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off) + if (!disk->l1) + goto error; + if (pread(disk->fd, disk->l1, 8*disk->l1sz, disk->l1off) != 8*disk->l1sz) { - free(disk->l1); - return -1; + log_warn("%s: unable to read qcow2 L1 table", __func__); + goto error; } for (i = 0; i < disk->l1sz; i++) disk->l1[i] = be64toh(disk->l1[i]); version = be32toh(header.version); if (version != 2 && version != 3) { log_warn("%s: unknown qcow2 version %d", __func__, version); - return -1; + goto error; } backingoff = be64toh(header.backingoff); @@ -223,34 +227,45 @@ qc2_open(struct qcdisk *disk, int fd) * otherwise we just crash with a pledge violation. */ log_warn("%s: unsupported external snapshot images", __func__); - return -1; + goto error; if (backingsz >= sizeof basepath - 1) { log_warn("%s: snapshot path too long", __func__); - return -1; + goto error; } if (pread(fd, basepath, backingsz, backingoff) != backingsz) { log_warn("%s: could not read snapshot base name", __func__); - return -1; + goto error; } basepath[backingsz] = 0; disk->base = calloc(1, sizeof(struct qcdisk)); if (qc2_openpath(disk->base, basepath, O_RDONLY) == -1) { - free(disk->base); - return -1; + log_warn("%s: could not open %s", basepath, __func__); + goto error; } if (disk->base->clustersz != disk->clustersz) { log_warn("%s: all disks must share clustersize", __func__); - free(disk->base); - return -1; + goto error; } } - fstat(fd, &st); + if (fstat(fd, &st) == -1) { + log_warn("%s: unable to stat disk", __func__); + goto error; + } + disk->end = st.st_size; + + log_debug("opened qcow2 disk version %d:", version); + log_debug("size:\t%lld", disk->disksz); + log_debug("end:\t%lld", disk->end); + log_debug("nsnap:\t%d", disk->nsnap); return 0; +error: + qc2_close(disk); + return -1; } static ssize_t @@ -362,8 +377,9 @@ qc2_close(void *p) struct qcdisk *disk; disk = p; - pwrite(disk->fd, disk->l1, disk->l1sz, disk->l1off); - close(disk->fd); + if (disk->base) + qc2_close(disk->base); + free(disk->l1); free(disk); } diff --git usr.sbin/vmd/vioraw.c usr.sbin/vmd/vioraw.c index 4b87c649407..9c76b1356f9 100644 --- usr.sbin/vmd/vioraw.c +++ usr.sbin/vmd/vioraw.c @@ -62,6 +62,8 @@ virtio_init_raw(struct virtio_backing *file, off_t *szp, int fd) return -1; fdp = malloc(sizeof(int)); + if (!fdp) + return -1; *fdp = fd; file->p = fdp; file->pread = raw_pread; diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c index c774cc7813a..20a94e6fa4d 100644 --- usr.sbin/vmd/virtio.c +++ usr.sbin/vmd/virtio.c @@ -2009,6 +2009,17 @@ virtio_init(struct vmd_vm *vm, int child_cdrom, int *child_disks, evtimer_set(&vmmci.timeout, vmmci_timeout, NULL); } +void +virtio_shutdown(struct vmd_vm *vm) +{ + int i; + + /* ensure that our disks are synced */ + vioscsi->file.close(vioscsi->file.p); + for (i = 0; i < nr_vioblk; i++) + vioblk[i].file.close(vioblk[i].file.p); +} + int vmmci_restore(int fd, uint32_t vm_id) { diff --git usr.sbin/vmd/virtio.h usr.sbin/vmd/virtio.h index 86ee6d21f9f..574f77d5fe8 100644 --- usr.sbin/vmd/virtio.h +++ usr.sbin/vmd/virtio.h @@ -258,6 +258,7 @@ struct ioinfo { /* virtio.c */ void virtio_init(struct vmd_vm *, int, int *, int *); +void virtio_shutdown(struct vmd_vm *); int virtio_dump(int); int virtio_restore(int, struct vmd_vm *, int, int *, int *); uint32_t vring_size(uint32_t); diff --git usr.sbin/vmd/vm.c usr.sbin/vmd/vm.c index 046b2be8503..491ca2eae3f 100644 --- usr.sbin/vmd/vm.c +++ usr.sbin/vmd/vm.c @@ -371,6 +371,9 @@ start_vm(struct vmd_vm *vm, int fd) /* Execute the vcpu run loop(s) for this VM */ ret = run_vm(vm->vm_cdrom, vm->vm_disks, nicfds, &vm->vm_params, &vrs); + /* Ensure that any in-flight data is written back */ + virtio_shutdown(vm); + return (ret); } -- Ori Bernstein
