On Wed, 24 Oct 2018 16:23:29 +0800, Michael Mikonos <m...@ii.net> wrote:
> On Tue, Oct 23, 2018 at 09:44:24PM -0700, Ori Bernstein wrote: > > This patch turns most warnings into errors, and uses the > > appropriate fatal/fatalx so that we don't print bogus error > > strings. It also adds checks for unsupported refcount sizes > > and writes that clobber the header. > > > > Ok? > > Hello, two comments below. Updated. Also changed some fatalx'es to fatals, so we get the errno, and removed status returns plus associated checks from functions that no longer had a failure path. diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c index 3a215599d49..12301650127 100644 --- usr.sbin/vmd/vioqcow2.c +++ usr.sbin/vmd/vioqcow2.c @@ -79,15 +79,15 @@ struct qcdisk { int fd; uint64_t *l1; off_t end; - uint32_t clustersz; + off_t clustersz; off_t disksz; /* In bytes */ - uint32_t cryptmethod; + uint32_t cryptmethod; uint32_t l1sz; off_t l1off; off_t refoff; - uint32_t refsz; + off_t refsz; uint32_t nsnap; off_t snapoff; @@ -102,9 +102,9 @@ struct qcdisk { extern char *__progname; static off_t xlate(struct qcdisk *, off_t, int *); -static int copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t); +static void copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t); +static void inc_refs(struct qcdisk *, off_t, int); static off_t mkcluster(struct qcdisk *, struct qcdisk *, off_t, off_t); -static int inc_refs(struct qcdisk *, off_t, int); static int qc2_open(struct qcdisk *, int *, size_t); static ssize_t qc2_pread(void *, char *, size_t, off_t); static ssize_t qc2_pwrite(void *, char *, size_t, off_t); @@ -137,6 +137,10 @@ virtio_init_qcow2(struct virtio_backing *file, off_t *szp, int *fd, size_t nfd) return 0; } +/* + * Return the path to the base image given a disk image. + * Called from vmctl. + */ ssize_t virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath) { @@ -151,7 +155,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath) return -1; } if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) { - log_warn("%s: invalid magic numbers", __func__); + log_warnx("%s: invalid magic numbers", __func__); return -1; } backingoff = be64toh(header.backingoff); @@ -160,7 +164,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath) return 0; if (backingsz >= npath - 1) { - log_warn("%s: snapshot path too long", __func__); + log_warnx("%s: snapshot path too long", __func__); return -1; } if (pread(fd, path, backingsz, backingoff) != backingsz) { @@ -178,20 +182,19 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath) if (path[0] == '/') { if (realpath(path, expanded) == NULL || strlcpy(path, expanded, npath) >= npath) { - log_warn("unable to resolve %s", path); + log_warnx("unable to resolve %s", path); return -1; } } else { s = dirname(dpath); if (snprintf(expanded, sizeof(expanded), "%s/%s", s, path) >= (int)sizeof(expanded)) { - log_warn("path too long: %s/%s", - s, path); + log_warnx("path too long: %s/%s", s, path); return -1; } if (npath < PATH_MAX || realpath(expanded, path) == NULL) { - log_warn("unable to resolve %s", path); + log_warnx("unable to resolve %s", path); return -1; } } @@ -207,7 +210,7 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd) struct qcheader header; uint64_t backingoff; uint32_t backingsz; - size_t i; + off_t i; int version, fd; pthread_rwlock_init(&disk->lock, NULL); @@ -216,15 +219,10 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd) disk->base = NULL; disk->l1 = NULL; - if (pread(fd, &header, sizeof(header), 0) != sizeof(header)) { - log_warn("%s: short read on header", __func__); - goto error; - } - if (strncmp(header.magic, - VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) { - log_warn("%s: invalid magic numbers", __func__); - goto error; - } + if (pread(fd, &header, sizeof(header), 0) != sizeof(header)) + fatalx("%s: short read on header", __func__); + if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) + fatalx("%s: invalid magic numbers", __func__); disk->clustersz = (1ull << be32toh(header.clustershift)); disk->disksz = be64toh(header.disksz); @@ -249,79 +247,59 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd) /* * We only know about the dirty or corrupt bits here. */ - if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) { - log_warnx("%s: unsupported features %llx", __func__, + if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) + fatalx("%s: unsupported features %llx", __func__, disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)); - goto error; - } + if (be32toh(header.reforder) != 4) + fatalx("%s: unsupported refcount size\n", __func__); disk->l1 = calloc(disk->l1sz, sizeof(*disk->l1)); if (!disk->l1) - goto error; + fatalx("%s: could not allocate l1 table", __func__); if (pread(disk->fd, disk->l1, 8 * disk->l1sz, disk->l1off) - != 8 * disk->l1sz) { - log_warn("%s: unable to read qcow2 L1 table", __func__); - goto error; - } + != 8 * disk->l1sz) + fatalx("%s: unable to read qcow2 L1 table", __func__); 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); - goto error; - } + if (version != 2 && version != 3) + fatalx("%s: unknown qcow2 version %d", __func__, version); backingoff = be64toh(header.backingoff); backingsz = be32toh(header.backingsz); if (backingsz != 0) { if (backingsz >= sizeof(basepath) - 1) { - log_warn("%s: snapshot path too long", __func__); - goto error; + fatalx("%s: snapshot path too long", __func__); } if (pread(fd, basepath, backingsz, backingoff) != backingsz) { - log_warn("%s: could not read snapshot base name", + fatalx("%s: could not read snapshot base name", __func__); - goto error; } basepath[backingsz] = 0; if (nfd <= 1) { - log_warnx("%s: missing base image %s", __func__, + fatalx("%s: missing base image %s", __func__, basepath); - goto error; } disk->base = calloc(1, sizeof(struct qcdisk)); if (!disk->base) - goto error; - if (qc2_open(disk->base, fds + 1, nfd - 1) == -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", + fatal("%s: could not open %s", basepath, __func__); + if (qc2_open(disk->base, fds + 1, nfd - 1) == -1) + fatalx("%s: could not open %s", basepath, __func__); + if (disk->base->clustersz != disk->clustersz) + fatalx("%s: all disk parts must share clustersize", __func__); - goto error; - } - } - if (fstat(fd, &st) == -1) { - log_warn("%s: unable to stat disk", __func__); - goto error; } + if (fstat(fd, &st) == -1) + fatal("%s: unable to stat disk", __func__); disk->end = st.st_size; log_debug("%s: qcow2 disk version %d size %lld end %lld snap %d", - __func__, - version, - disk->disksz, - disk->end, - disk->nsnap); + __func__, version, disk->disksz, disk->end, disk->nsnap); return 0; -error: - qc2_close(disk, 0); - return -1; } static ssize_t @@ -418,6 +396,8 @@ qc2_pwrite(void *p, char *buf, size_t len, off_t off) phys_off = mkcluster(disk, d, off, phys_off); if (phys_off == -1) return -1; + if (phys_off < disk->clustersz) + fatalx("writing reserved cluster"); if (pwrite(disk->fd, buf, sz, phys_off) != sz) return -1; off += sz; @@ -487,10 +467,8 @@ xlate(struct qcdisk *disk, off_t off, int *inplace) */ if (inplace) *inplace = !!(cluster & QCOW2_INPLACE); - if (cluster & QCOW2_COMPRESSED) { - log_warn("%s: compressed clusters unsupported", __func__); - goto err; - } + if (cluster & QCOW2_COMPRESSED) + fatalx("%s: compressed clusters unsupported", __func__); pthread_rwlock_unlock(&disk->lock); clusteroff = 0; cluster &= ~QCOW2_INPLACE; @@ -525,13 +503,8 @@ mkcluster(struct qcdisk *disk, struct qcdisk *base, off_t off, off_t src_phys) l2sz = disk->clustersz / 8; l1off = off / (disk->clustersz * l2sz); if (l1off >= disk->l1sz) - goto fail; + fatalx("l1 offset outside disk"); - /* - * Align disk to cluster size, for ftruncate: Not strictly - * required, but it easier to eyeball buggy write offsets, - * and helps performance a bit. - */ disk->end = (disk->end + disk->clustersz - 1) & ~(disk->clustersz - 1); l2tab = disk->l1[l1off]; @@ -541,75 +514,63 @@ mkcluster(struct qcdisk *disk, struct qcdisk *base, off_t off, off_t src_phys) orig = l2tab & ~QCOW2_INPLACE; l2tab = disk->end; disk->end += disk->clustersz; - if (ftruncate(disk->fd, disk->end) == -1) { - perror("ftruncate"); - goto fail; - } + if (ftruncate(disk->fd, disk->end) == -1) + fatal("%s: ftruncate failed", __func__); /* * If we translated, found a L2 entry, but it needed to * be copied, copy it. */ - if (orig != 0 && copy_cluster(disk, disk, l2tab, orig) == -1) { - perror("move cluster"); - goto fail; - } + if (orig != 0) + copy_cluster(disk, disk, l2tab, orig); /* Update l1 -- we flush it later */ disk->l1[l1off] = l2tab | QCOW2_INPLACE; - if (inc_refs(disk, l2tab, 1) == -1) { - perror("refs"); - goto fail; - } + inc_refs(disk, l2tab, 1); } l2tab &= ~QCOW2_INPLACE; /* Grow the disk */ if (ftruncate(disk->fd, disk->end + disk->clustersz) < 0) - goto fail; + fatalx("%s: could not grow disk", __func__); if (src_phys > 0) - if (copy_cluster(disk, base, disk->end, src_phys) == -1) - goto fail; + copy_cluster(disk, base, disk->end, src_phys); cluster = disk->end; disk->end += disk->clustersz; buf = htobe64(cluster | QCOW2_INPLACE); if (pwrite(disk->fd, &buf, sizeof(buf), l2tab + l2off * 8) != 8) - goto fail; + fatalx("%s: could not write cluster", __func__); /* TODO: lazily sync: currently VMD doesn't close things */ buf = htobe64(disk->l1[l1off]); if (pwrite(disk->fd, &buf, sizeof(buf), disk->l1off + 8 * l1off) != 8) - goto fail; - if (inc_refs(disk, cluster, 1) == -1) - goto fail; + fatalx("%s: could not write l1", __func__); + inc_refs(disk, cluster, 1); pthread_rwlock_unlock(&disk->lock); clusteroff = off % disk->clustersz; + if (cluster + clusteroff < disk->clustersz) + fatalx("write would clobber header"); return cluster + clusteroff; - -fail: - pthread_rwlock_unlock(&disk->lock); - return -1; } /* Copies a cluster containing src to dst. Src and dst need not be aligned. */ -static int +static void copy_cluster(struct qcdisk *disk, struct qcdisk *base, off_t dst, off_t src) { char *scratch; scratch = alloca(disk->clustersz); if (!scratch) - err(1, "out of memory"); + fatal("out of memory"); src &= ~(disk->clustersz - 1); dst &= ~(disk->clustersz - 1); if (pread(base->fd, scratch, disk->clustersz, src) == -1) - return -1; + fatal("%s: could not read cluster", __func__); if (pwrite(disk->fd, scratch, disk->clustersz, dst) == -1) - return -1; - return 0; + fatal("%s: could not write cluster", __func__); } -static int +static void inc_refs(struct qcdisk *disk, off_t off, int newcluster) { off_t l1off, l1idx, l2idx, l2cluster; @@ -623,34 +584,28 @@ inc_refs(struct qcdisk *disk, off_t off, int newcluster) l2idx = (off / disk->clustersz) % nper; l1off = disk->refoff + 8 * l1idx; if (pread(disk->fd, &buf, sizeof(buf), l1off) != 8) - return -1; + fatal("could not read refs"); l2cluster = be64toh(buf); if (l2cluster == 0) { l2cluster = disk->end; disk->end += disk->clustersz; - if (ftruncate(disk->fd, disk->end) < 0) { - log_warn("%s: refs block grow fail", __func__); - return -1; - } + if (ftruncate(disk->fd, disk->end) < 0) + fatal("%s: failed to allocate ref block", __func__); buf = htobe64(l2cluster); - if (pwrite(disk->fd, &buf, sizeof(buf), l1off) != 8) { - return -1; - } + if (pwrite(disk->fd, &buf, sizeof(buf), l1off) != 8) + fatal("%s: failed to write ref block", __func__); } refs = 1; if (!newcluster) { if (pread(disk->fd, &refs, sizeof(refs), l2cluster + 2 * l2idx) != 2) - return -1; + fatal("could not read ref cluster"); refs = be16toh(refs) + 1; } refs = htobe16(refs); - if (pwrite(disk->fd, &refs, sizeof(refs), l2cluster + 2 * l2idx) != 2) { - log_warn("%s: could not write ref block", __func__); - return -1; - } - return 0; + if (pwrite(disk->fd, &refs, sizeof(refs), l2cluster + 2 * l2idx) != 2) + fatal("%s: could not write ref block", __func__); } -- Ori Bernstein