futility/host lib: Fix coverity warnings

Assorted minor code issues, which we should fix so any new errors stand
out more.

BUG=chromium:643769
BRANCH=none
TEST=make runtests

Change-Id: Ib37b45dea54bd506b519b0304300b8d192e34339
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/382319
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>

15 files changed, 140 insertions, 89 deletions

diff --git a/futility/cmd_create.c b/futility/cmd_create.c
index c8219511..578e3be5 100644
--- a/futility/cmd_create.c
+++ b/futility/cmd_create.c
@@ -56,7 +56,7 @@ static const struct option long_opts[] = {
 
 static void print_help(int argc, char *argv[])
 {
-	struct vb2_text_vs_enum *entry;
+	const struct vb2_text_vs_enum *entry;
 
 	printf("\n"
 "Usage:  " MYNAME " %s [options] <INFILE> [<BASENAME>]\n", argv[0]);
diff --git a/futility/cmd_dump_fmap.c b/futility/cmd_dump_fmap.c
index b512fdca..89b702bd 100644
--- a/futility/cmd_dump_fmap.c
+++ b/futility/cmd_dump_fmap.c
@@ -39,9 +39,10 @@ static int normal_fmap(const FmapHeader *fmh, int argc, char *argv[])
 	char *extract_names[argc];
 	char *outname = 0;
 
+	memset(extract_names, 0, sizeof(extract_names));
+
 	if (opt_extract) {
 		/* prepare the filenames to write areas to */
-		memset(extract_names, 0, sizeof(extract_names));
 		for (i = 0; i < argc; i++) {
 			char *a = argv[i];
 			char *f = strchr(a, ':');
@@ -140,7 +141,8 @@ static int normal_fmap(const FmapHeader *fmh, int argc, char *argv[])
 				if (FMT_NORMAL == opt_format)
 					printf("saved as \"%s\"\n", outname);
 			}
-			fclose(fp);
+			if (fp)
+				fclose(fp);
 		}
 	}
 
diff --git a/futility/cmd_gbb_utility.c b/futility/cmd_gbb_utility.c
index fe21762e..f75cceb0 100644
--- a/futility/cmd_gbb_utility.c
+++ b/futility/cmd_gbb_utility.c
@@ -235,9 +235,10 @@ static uint8_t *read_entire_file(const char *filename, off_t *sizeptr)
 		goto fail;
 	}
 
-	if (fp && 0 != fclose(fp)) {
+	if (0 != fclose(fp)) {
 		fprintf(stderr, "ERROR: Unable to close %s: %s\n",
 			filename, strerror(errno));
+		fp = NULL;  /* Don't try to close it again */
 		goto fail;
 	}
 
diff --git a/futility/cmd_sign.c b/futility/cmd_sign.c
index e91a1a7b..cd46c8ba 100644
--- a/futility/cmd_sign.c
+++ b/futility/cmd_sign.c
@@ -425,7 +425,7 @@ static void print_help_kern_preamble(int argc, char *argv[])
 
 static void print_help_usbpd1(int argc, char *argv[])
 {
-	struct vb2_text_vs_enum *entry;
+	const struct vb2_text_vs_enum *entry;
 
 	printf("\n"
 	       "Usage:  " MYNAME " %s --type %s [options] INFILE [OUTFILE]\n"
diff --git a/futility/cmd_vbutil_firmware.c b/futility/cmd_vbutil_firmware.c
index 950690a0..2afb29f9 100644
--- a/futility/cmd_vbutil_firmware.c
+++ b/futility/cmd_vbutil_firmware.c
@@ -89,70 +89,73 @@ static int do_vblock(const char *outfile, const char *keyblock_file,
 		     const char *fv_file, const char *kernelkey_file,
 		     uint32_t preamble_flags)
 {
+	struct vb2_keyblock *keyblock = NULL;
+	struct vb2_private_key *signing_key = NULL;
+	struct vb2_packed_key *kernel_subkey = NULL;
+	struct vb2_signature *body_sig = NULL;
+	struct vb2_fw_preamble *preamble = NULL;
+	uint8_t *fv_data = NULL;
+	int retval = 1;
+
 	if (!outfile) {
 		VbExError("Must specify output filename\n");
-		return 1;
+		goto vblock_cleanup;
 	}
 	if (!keyblock_file || !signprivate || !kernelkey_file) {
 		VbExError("Must specify all keys\n");
-		return 1;
+		goto vblock_cleanup;
 	}
 	if (!fv_file) {
 		VbExError("Must specify firmware volume\n");
-		return 1;
+		goto vblock_cleanup;
 	}
 
 	/* Read the key block and keys */
-	struct vb2_keyblock *keyblock = vb2_read_keyblock(keyblock_file);
+	keyblock = vb2_read_keyblock(keyblock_file);
 	if (!keyblock) {
 		VbExError("Error reading key block.\n");
-		return 1;
+		goto vblock_cleanup;
 	}
 
-	struct vb2_private_key *signing_key = vb2_read_private_key(signprivate);
+	signing_key = vb2_read_private_key(signprivate);
 	if (!signing_key) {
 		VbExError("Error reading signing key.\n");
-		return 1;
+		goto vblock_cleanup;
 	}
 
-	struct vb2_packed_key *kernel_subkey =
-		vb2_read_packed_key(kernelkey_file);
+	kernel_subkey = vb2_read_packed_key(kernelkey_file);
 	if (!kernel_subkey) {
 		VbExError("Error reading kernel subkey.\n");
-		return 1;
+		goto vblock_cleanup;
 	}
 
 	/* Read and sign the firmware volume */
-	uint8_t *fv_data;
 	uint32_t fv_size;
 	if (VB2_SUCCESS != vb2_read_file(fv_file, &fv_data, &fv_size))
-		return 1;
+		goto vblock_cleanup;
 	if (!fv_size) {
 		VbExError("Empty firmware volume file\n");
-		return 1;
+		goto vblock_cleanup;
 	}
-	struct vb2_signature *body_sig =
-		vb2_calculate_signature(fv_data, fv_size, signing_key);
+	body_sig = vb2_calculate_signature(fv_data, fv_size, signing_key);
 	if (!body_sig) {
 		VbExError("Error calculating body signature\n");
-		return 1;
+		goto vblock_cleanup;
 	}
-	free(fv_data);
 
 	/* Create preamble */
-	struct vb2_fw_preamble *preamble =
-		vb2_create_fw_preamble(version, kernel_subkey, body_sig,
-				       signing_key, preamble_flags);
+	preamble = vb2_create_fw_preamble(version, kernel_subkey, body_sig,
+					  signing_key, preamble_flags);
 	if (!preamble) {
 		VbExError("Error creating preamble.\n");
-		return 1;
+		goto vblock_cleanup;
 	}
 
 	/* Write the output file */
 	FILE *f = fopen(outfile, "wb");
 	if (!f) {
 		VbExError("Can't open output file %s\n", outfile);
-		return 1;
+		goto vblock_cleanup;
 	}
 	int i = ((1 != fwrite(keyblock, keyblock->keyblock_size, 1, f)) ||
 		 (1 != fwrite(preamble, preamble->preamble_size, 1, f)));
@@ -160,11 +163,27 @@ static int do_vblock(const char *outfile, const char *keyblock_file,
 	if (i) {
 		VbExError("Can't write output file %s\n", outfile);
 		unlink(outfile);
-		return 1;
+		goto vblock_cleanup;
 	}
 
 	/* Success */
-	return 0;
+	retval = 0;
+
+vblock_cleanup:
+	if (keyblock)
+		free(keyblock);
+	if (signing_key)
+		free(signing_key);
+	if (kernel_subkey)
+		free(kernel_subkey);
+	if (fv_data)
+		free(fv_data);
+	if (body_sig)
+		free(body_sig);
+	if (preamble)
+		free(preamble);
+
+	return retval;
 }
 
 static int do_verify(const char *infile, const char *signpubkey,
@@ -176,38 +195,40 @@ static int do_verify(const char *infile, const char *signpubkey,
 
 	uint32_t now = 0;
 
+	uint8_t *pubkbuf = NULL;
+	uint8_t *blob = NULL;
+	uint8_t *fv_data = NULL;
+	int retval = 1;
+
 	if (!infile || !signpubkey || !fv_file) {
 		VbExError("Must specify filename, signpubkey, and fv\n");
-		return 1;
+		goto verify_cleanup;
 	}
 
 	/* Read public signing key */
-	uint8_t *pubkbuf;
 	uint32_t pubklen;
 	struct vb2_public_key sign_key;
 	if (VB2_SUCCESS != vb2_read_file(signpubkey, &pubkbuf, &pubklen)) {
 		fprintf(stderr, "Error reading signpubkey.\n");
-		return 1;
+		goto verify_cleanup;
 	}
 	if (VB2_SUCCESS != vb2_unpack_key(&sign_key, pubkbuf, pubklen)) {
 		fprintf(stderr, "Error unpacking signpubkey.\n");
-		return 1;
+		goto verify_cleanup;
 	}
 
 	/* Read blob */
-	uint8_t *blob;
 	uint32_t blob_size;
 	if (VB2_SUCCESS != vb2_read_file(infile, &blob, &blob_size)) {
 		VbExError("Error reading input file\n");
-		return 1;
+		goto verify_cleanup;
 	}
 
 	/* Read firmware volume */
-	uint8_t *fv_data;
 	uint32_t fv_size;
 	if (VB2_SUCCESS != vb2_read_file(fv_file, &fv_data, &fv_size)) {
 		VbExError("Error reading firmware volume\n");
-		return 1;
+		goto verify_cleanup;
 	}
 
 	/* Verify key block */
@@ -215,9 +236,8 @@ static int do_verify(const char *infile, const char *signpubkey,
 	if (VB2_SUCCESS !=
 	    vb2_verify_keyblock(keyblock, blob_size, &sign_key, &wb)) {
 		VbExError("Error verifying key block.\n");
-		return 1;
+		goto verify_cleanup;
 	}
-	free(pubkbuf);
 
 	now += keyblock->keyblock_size;
 
@@ -239,7 +259,7 @@ static int do_verify(const char *infile, const char *signpubkey,
 			   keyblock->data_key.key_offset +
 			   keyblock->data_key.key_size)) {
 		fprintf(stderr, "Error parsing data key.\n");
-		return 1;
+		goto verify_cleanup;
 	}
 
 	/* Verify preamble */
@@ -247,7 +267,7 @@ static int do_verify(const char *infile, const char *signpubkey,
 	if (VB2_SUCCESS !=
 	    vb2_verify_fw_preamble(pre2, blob_size - now, &data_key, &wb)) {
 		VbExError("Error2 verifying preamble.\n");
-		return 1;
+		goto verify_cleanup;
 	}
 	now += pre2->preamble_size;
 
@@ -282,17 +302,28 @@ static int do_verify(const char *infile, const char *signpubkey,
 		printf("Body verification succeeded.\n");
 	} else {
 		VbExError("Error verifying firmware body.\n");
-		return 1;
+		goto verify_cleanup;
 	}
 
 	if (kernelkey_file &&
 	    VB2_SUCCESS != vb2_write_packed_key(kernelkey_file,
 						kernel_subkey)) {
 		VbExError("Unable to write kernel subkey\n");
-		return 1;
+		goto verify_cleanup;
 	}
 
-	return 0;
+	/* Success */
+	retval = 0;
+
+verify_cleanup:
+	if (pubkbuf)
+		free(pubkbuf);
+	if (blob)
+		free(blob);
+	if (fv_data)
+		free(fv_data);
+
+	return retval;
 }
 
 static int do_vbutil_firmware(int argc, char *argv[])
diff --git a/futility/cmd_vbutil_kernel.c b/futility/cmd_vbutil_kernel.c
index bdc11517..77276d34 100644
--- a/futility/cmd_vbutil_kernel.c
+++ b/futility/cmd_vbutil_kernel.c
@@ -213,6 +213,7 @@ static uint8_t *ReadOldKPartFromFileOrDie(const char *filename,
 	if (size_ptr)
 		*size_ptr = file_size;
 
+	fclose(fp);
 	return buf;
 }
 
diff --git a/futility/cmd_vbutil_key.c b/futility/cmd_vbutil_key.c
index 58f724c7..4362255d 100644
--- a/futility/cmd_vbutil_key.c
+++ b/futility/cmd_vbutil_key.c
@@ -97,6 +97,7 @@ static int do_pack(const char *infile, const char *outfile, uint32_t algorithm,
 	if (privkey) {
 		if (VB2_SUCCESS != vb2_write_private_key(outfile, privkey)) {
 			fprintf(stderr, "vbutil_key: Error writing key.\n");
+			free(privkey);
 			return 1;
 		}
 		free(privkey);
diff --git a/futility/file_type.c b/futility/file_type.c
index dff5d29d..0b45be18 100644
--- a/futility/file_type.c
+++ b/futility/file_type.c
@@ -119,6 +119,7 @@ enum futil_file_err futil_file_type(const char *filename,
 	if (0 != fstat(ifd, &sb)) {
 		fprintf(stderr, "Can't stat input file: %s\n",
 			strerror(errno));
+		close(ifd);
 		return FILE_ERR_STAT;
 	}
 
diff --git a/host/arch/x86/lib/crossystem_arch.c b/host/arch/x86/lib/crossystem_arch.c
index d7e89fe1..2b5d00aa 100644
--- a/host/arch/x86/lib/crossystem_arch.c
+++ b/host/arch/x86/lib/crossystem_arch.c
@@ -170,9 +170,9 @@ int VbReadNvStorage(VbNvContext* vnc) {
 }
 
 
-int VbWriteNvStorage(VbNvContext* vnc) {
+int VbWriteNvStorage(VbNvContext* vnc)
+{
   unsigned offs, blksz;
-  VbSharedDataHeader *sh = VbSharedDataRead();
 
   if (!vnc->raw_changed)
     return 0;  /* Nothing changed, so no need to write */
@@ -189,8 +189,12 @@ int VbWriteNvStorage(VbNvContext* vnc) {
     return -1;
 
   /* Also attempt to write using mosys if using vboot2 */
-  if (sh && (sh->flags & VBSD_BOOT_FIRMWARE_VBOOT2))
-    VbWriteNvStorage_mosys(vnc);
+  VbSharedDataHeader *sh = VbSharedDataRead();
+  if (sh) {
+    if (sh->flags & VBSD_BOOT_FIRMWARE_VBOOT2)
+      VbWriteNvStorage_mosys(vnc);
+    free(sh);
+  }
 
   return 0;
 }
diff --git a/host/lib/crossystem.c b/host/lib/crossystem.c
index 87c74169..53ec6897 100644
--- a/host/lib/crossystem.c
+++ b/host/lib/crossystem.c
@@ -755,7 +755,7 @@ static int InAndroid() {
      check if file exists.  Using fstat because for some
      reason, stat() was seg faulting in Android */
   fd = open(MOSYS_ANDROID_PATH, O_RDONLY);
-  if (fstat(fd, &s) == 0) {
+  if (fd != -1 && fstat(fd, &s) == 0) {
     close(fd);
     return 1;
   }
diff --git a/host/lib/file_keys.c b/host/lib/file_keys.c
index fd07752b..a774ba22 100644
--- a/host/lib/file_keys.c
+++ b/host/lib/file_keys.c
@@ -22,45 +22,52 @@
 #include "host_common.h"
 #include "signature_digest.h"
 
-uint8_t* BufferFromFile(const char* input_file, uint64_t* len) {
-  int fd;
-  struct stat stat_fd;
-  uint8_t* buf = NULL;
+uint8_t *BufferFromFile(const char* input_file, uint64_t* len)
+{
+	int fd;
+	struct stat stat_fd;
+	uint8_t* buf = NULL;
 
-  if ((fd = open(input_file, O_RDONLY)) == -1) {
-    VBDEBUG(("Couldn't open file %s\n", input_file));
-    return NULL;
-  }
+	if ((fd = open(input_file, O_RDONLY)) == -1) {
+		VBDEBUG(("Couldn't open file %s\n", input_file));
+		return NULL;
+	}
 
-  if (-1 == fstat(fd, &stat_fd)) {
-    VBDEBUG(("Couldn't stat file %s\n", input_file));
-    return NULL;
-  }
-  *len = stat_fd.st_size;
+	if (-1 == fstat(fd, &stat_fd)) {
+		VBDEBUG(("Couldn't stat file %s\n", input_file));
+		close(fd);
+		return NULL;
+	}
+	*len = stat_fd.st_size;
 
-  buf = (uint8_t*)malloc(*len);
-  if (!buf) {
-    VbExError("Couldn't allocate %ld bytes for file %s\n", *len, input_file);
-    return NULL;
-  }
+	buf = (uint8_t *)malloc(*len);
+	if (!buf) {
+		VbExError("Couldn't allocate %ld bytes for file %s\n",
+			  *len, input_file);
+		close(fd);
+		return NULL;
+	}
 
-  if (*len != read(fd, buf, *len)) {
-    VBDEBUG(("Couldn't read file %s into a buffer\n", input_file));
-    return NULL;
-  }
+	if (*len != read(fd, buf, *len)) {
+		VBDEBUG(("Couldn't read file %s into a buffer\n", input_file));
+		free(buf);
+		close(fd);
+		return NULL;
+	}
 
-  close(fd);
-  return buf;
+	close(fd);
+	return buf;
 }
 
-RSAPublicKey* RSAPublicKeyFromFile(const char* input_file) {
-  uint64_t len;
-  RSAPublicKey* key = NULL;
-  uint8_t* buf = BufferFromFile(input_file, &len);
-  if (buf)
-    key = RSAPublicKeyFromBuf(buf, len);
-  free(buf);
-  return key;
+RSAPublicKey *RSAPublicKeyFromFile(const char *input_file) {
+	uint64_t len;
+	RSAPublicKey* key = NULL;
+
+	uint8_t *buf = BufferFromFile(input_file, &len);
+	if (buf)
+		key = RSAPublicKeyFromBuf(buf, len);
+	free(buf);
+	return key;
 }
 
 int DigestFile(char *input_file, enum vb2_hash_algorithm alg,
diff --git a/host/lib/host_keyblock.c b/host/lib/host_keyblock.c
index 21fa0995..dde8fe6c 100644
--- a/host/lib/host_keyblock.c
+++ b/host/lib/host_keyblock.c
@@ -92,6 +92,9 @@ struct vb2_keyblock *vb2_create_keyblock_external(
                 uint32_t flags,
                 const char *external_signer)
 {
+	if (!signing_key_pem_file || !data_key || !external_signer)
+		return NULL;
+
 	uint32_t signed_size = sizeof(struct vb2_keyblock) + data_key->key_size;
 	uint32_t sig_data_size = vb2_rsa_sig_size(algorithm);
 	uint32_t block_size =
@@ -101,8 +104,6 @@ struct vb2_keyblock *vb2_create_keyblock_external(
 	struct vb2_keyblock *h = (struct vb2_keyblock *)calloc(block_size, 1);
 	if (!h)
 		return NULL;
-	if (!signing_key_pem_file || !data_key || !external_signer)
-		return NULL;
 
 	uint8_t *data_key_dest = (uint8_t *)(h + 1);
 	uint8_t *block_chk_dest = data_key_dest + data_key->key_size;
diff --git a/host/lib/host_misc.c b/host/lib/host_misc.c
index 3fb9b24e..89a6c2f1 100644
--- a/host/lib/host_misc.c
+++ b/host/lib/host_misc.c
@@ -27,7 +27,7 @@ char* StrCopy(char* dest, const char* src, int dest_size) {
 uint8_t* ReadFile(const char* filename, uint64_t* sizeptr) {
   FILE* f;
   uint8_t* buf;
-  uint64_t size;
+  long size;
 
   f = fopen(filename, "rb");
   if (!f) {
@@ -37,6 +37,8 @@ uint8_t* ReadFile(const char* filename, uint64_t* sizeptr) {
 
   fseek(f, 0, SEEK_END);
   size = ftell(f);
+  if (size < 0)
+    return NULL;
   rewind(f);
 
   buf = malloc(size);
diff --git a/host/lib21/host_key.c b/host/lib21/host_key.c
index 4ef18d88..2ebd08a1 100644
--- a/host/lib21/host_key.c
+++ b/host/lib21/host_key.c
@@ -18,7 +18,7 @@
 #include "host_key2.h"
 #include "host_misc.h"
 
-struct vb2_text_vs_enum vb2_text_vs_sig[] = {
+const struct vb2_text_vs_enum vb2_text_vs_sig[] = {
 	{"RSA1024", VB2_SIG_RSA1024},
 	{"RSA2048", VB2_SIG_RSA2048},
 	{"RSA4096", VB2_SIG_RSA4096},
@@ -26,7 +26,7 @@ struct vb2_text_vs_enum vb2_text_vs_sig[] = {
 	{0, 0}
 };
 
-struct vb2_text_vs_enum vb2_text_vs_hash[] = {
+const struct vb2_text_vs_enum vb2_text_vs_hash[] = {
 	{"SHA1",   VB2_HASH_SHA1},
 	{"SHA256", VB2_HASH_SHA256},
 	{"SHA512", VB2_HASH_SHA512},
@@ -155,7 +155,7 @@ int vb21_private_key_read(struct vb2_private_key **key_ptr,
 			  const char *filename)
 {
 	uint32_t size = 0;
-	uint8_t *buf;
+	uint8_t *buf = NULL;
 	int rv;
 
 	*key_ptr = NULL;
diff --git a/host/lib21/include/host_key2.h b/host/lib21/include/host_key2.h
index 4681a5f4..5a2ec0ad 100644
--- a/host/lib21/include/host_key2.h
+++ b/host/lib21/include/host_key2.h
@@ -55,8 +55,8 @@ const struct vb2_text_vs_enum *vb2_lookup_by_name(
 	const struct vb2_text_vs_enum *table,
 	const char *name);
 
-extern struct vb2_text_vs_enum vb2_text_vs_sig[];
-extern struct vb2_text_vs_enum vb2_text_vs_hash[];
+extern const struct vb2_text_vs_enum vb2_text_vs_sig[];
+extern const struct vb2_text_vs_enum vb2_text_vs_hash[];
 
 /**
  * Return the name of a signature algorithm.