On Tue, Sep 11, 2018 at 03:36:49PM +0800, Michael Mikonos 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?
I think the previous patch was too simple minded. Please see new
patch below.
In elf{32,64}_exec() phdr is freed before anything else is
allocated, so later error blocks don't need to free it.
shp is allocated before shstr so free(shp) if allocating shstr
fails.
In qc2_open() disk is passed as a parameter and disk->l1 is
only free()d if pread() fails. Zero the disk->l1 and disk->base
pointers if they are free()d on error.
Index: loadfile_elf.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/loadfile_elf.c,v
retrieving revision 1.30
diff -u -p -u -r1.30 loadfile_elf.c
--- loadfile_elf.c 17 Jul 2018 13:47:06 -0000 1.30
+++ loadfile_elf.c 11 Sep 2018 12:22:36 -0000
@@ -716,6 +716,8 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
sz = elf->e_phnum * sizeof(Elf64_Phdr);
phdr = malloc(sz);
+ if (phdr == NULL)
+ return 1;
if (fseeko(fp, (off_t)elf->e_phoff, SEEK_SET) == -1) {
free(phdr);
@@ -813,6 +815,8 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
}
sz = elf->e_shnum * sizeof(Elf64_Shdr);
shp = malloc(sz);
+ if (shp == NULL)
+ return 1;
if (fread(shp, 1, sz, fp) != sz) {
free(shp);
@@ -824,6 +828,10 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
size_t shstrsz = shp[elf->e_shstrndx].sh_size;
char *shstr = malloc(shstrsz);
+ if (shstr == NULL) {
+ free(shp);
+ return 1;
+ }
if (fseeko(fp, (off_t)shp[elf->e_shstrndx].sh_offset,
SEEK_SET) == -1) {
free(shstr);
@@ -938,6 +946,8 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
sz = elf->e_phnum * sizeof(Elf32_Phdr);
phdr = malloc(sz);
+ if (phdr == NULL)
+ return 1;
if (fseeko(fp, (off_t)elf->e_phoff, SEEK_SET) == -1) {
free(phdr);
@@ -1035,6 +1045,8 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
}
sz = elf->e_shnum * sizeof(Elf32_Shdr);
shp = malloc(sz);
+ if (shp == NULL)
+ return 1;
if (fread(shp, 1, sz, fp) != sz) {
free(shp);
@@ -1046,6 +1058,10 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
size_t shstrsz = shp[elf->e_shstrndx].sh_size;
char *shstr = malloc(shstrsz);
+ if (shstr == NULL) {
+ free(shp);
+ return 1;
+ }
if (fseeko(fp, (off_t)shp[elf->e_shstrndx].sh_offset,
SEEK_SET) == -1) {
free(shstr);
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 12:22:36 -0000
@@ -202,9 +202,13 @@ 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);
+ disk->l1 = NULL;
return -1;
}
for (i = 0; i < disk->l1sz; i++)
@@ -237,14 +241,18 @@ qc2_open(struct qcdisk *disk, int fd)
basepath[backingsz] = 0;
disk->base = calloc(1, sizeof(struct qcdisk));
+ if (disk->base == NULL)
+ return -1;
if (qc2_openpath(disk->base, basepath, O_RDONLY) == -1) {
free(disk->base);
+ disk->base = NULL;
return -1;
}
if (disk->base->clustersz != disk->clustersz) {
log_warn("%s: all disks must share clustersize",
__func__);
free(disk->base);
+ disk->base = NULL;
return -1;
}
}
Index: vioraw.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/vioraw.c,v
retrieving revision 1.1
diff -u -p -u -r1.1 vioraw.c
--- vioraw.c 25 Aug 2018 04:16:09 -0000 1.1
+++ vioraw.c 11 Sep 2018 12:22:36 -0000
@@ -62,6 +62,8 @@ virtio_init_raw(struct virtio_backing *f
return -1;
fdp = malloc(sizeof(int));
+ if (fdp == NULL)
+ return -1;
*fdp = fd;
file->p = fdp;
file->pread = raw_pread;
