From 9a3a32ebac0a2c4406cb2bdafedad237069a9b0b Mon Sep 17 00:00:00 2001
From: Alberto Leiva Popper <ydahhrk@gmail.com>
Date: Tue, 12 Feb 2019 15:44:06 -0600
Subject: [PATCH] Add a bunch of GCC warning flags

Found lots of bugs because of them. Most are fixed.
---
 src/Makefile.am          | 77 +++++++++++++++++++++++++++++++++++++++-
 src/address.c            |  6 ++--
 src/asn1/decode.c        |  2 +-
 src/asn1/signed_data.c   |  5 ++-
 src/common.c             |  8 +++--
 src/debug.c              |  5 +--
 src/extension.c          |  4 +--
 src/extension.h          |  5 ++-
 src/log.h                | 28 ++++++++++-----
 src/object/certificate.c | 21 +++++------
 src/object/crl.c         |  6 ++--
 src/resource.c           | 10 +++---
 src/sorted_array.c       |  4 +--
 13 files changed, 133 insertions(+), 48 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 5379704e..c9637466 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1,4 +1,5 @@
 # Comment these out during releases.
+# Also increase optimization I guess (-O0 -> -On)
 CFLAGS_DEBUG  = -DDEBUG
 LDFLAGS_DEBUG = -rdynamic
 
@@ -47,5 +48,79 @@ rpki_validator_SOURCES += resource/ip4.h resource/ip4.c
 rpki_validator_SOURCES += resource/ip6.h resource/ip6.c
 rpki_validator_SOURCES += resource/asn.h resource/asn.c
 
-rpki_validator_CFLAGS  = -pedantic -Wall -std=gnu11 -O0 -g $(CFLAGS_DEBUG)
+rpki_validator_CFLAGS  = -Wall
+# Feel free to temporarily remove this one if you're not using gcc 7.3.0.
+rpki_validator_CFLAGS += $(GCC_WARNS)
+rpki_validator_CFLAGS += -std=gnu11 -O0 -g $(CFLAGS_DEBUG)
 rpki_validator_LDFLAGS = $(LDFLAGS_DEBUG)
+
+# I'm tired of scrolling up, but feel free to comment this out.
+GCC_WARNS  = -fmax-errors=1
+
+# Please ready some arguments if you want to permanently remove some of these
+# flags. I gave a quick read to the documentation of all of these warnings, and
+# generally liked each of them.
+GCC_WARNS += -Wpedantic -pedantic-errors -Waddress -Walloc-zero -Walloca
+GCC_WARNS += -Wno-aggressive-loop-optimizations -Warray-bounds=2 -Wbool-compare
+GCC_WARNS += -Wbool-operation -Wno-builtin-declaration-mismatch -Wcast-align
+GCC_WARNS += -Wcast-qual -Wchar-subscripts -Wchkp -Wclobbered -Wcomment
+GCC_WARNS += -Wdangling-else -Wdate-time -Wdisabled-optimization
+GCC_WARNS += -Wdouble-promotion -Wduplicated-branches -Wduplicated-cond
+GCC_WARNS += -Wempty-body -Wenum-compare -Wexpansion-to-defined -Wfloat-equal
+GCC_WARNS += -Wformat -Wformat-nonliteral -Wformat-overflow=2 -Wformat-security
+GCC_WARNS += -Wformat-signedness -Wformat-truncation=2 -Wformat-y2k
+GCC_WARNS += -Wframe-address -Wjump-misses-init -Wignored-qualifiers
+GCC_WARNS += -Wignored-attributes -Wincompatible-pointer-types
+
+# This is a fun one. Write "/* fallthrough */" to prevent a warning whenever
+# switch cases do not break.
+GCC_WARNS += -Wimplicit-fallthrough
+
+GCC_WARNS += -Wimplicit-function-declaration -Wimplicit-int -Winit-self -Winline
+GCC_WARNS += -Wint-in-bool-context -Winvalid-memory-model -Winvalid-pch
+GCC_WARNS += -Wlogical-op -Wlogical-not-parentheses -Wlong-long -Wmain
+GCC_WARNS += -Wmaybe-uninitialized -Wmemset-elt-size -Wmemset-transposed-args
+GCC_WARNS += -Wmisleading-indentation -Wmissing-braces -Wmissing-include-dirs
+GCC_WARNS += -Wnonnull -Wnonnull-compare -Wnormalized -Wnull-dereference
+
+# This one seems to be undocumented.
+GCC_WARNS += -Wodr
+
+# "Warn if the vectorizer cost model overrides the OpenMP or the Cilk Plus simd
+# directive set by user."
+# ... What?
+GCC_WARNS += -Wopenmp-simd
+
+GCC_WARNS += -Woverride-init-side-effects -Woverlength-strings -Wpacked
+GCC_WARNS += -Wpacked-bitfield-compat -Wparentheses -Wpointer-arith
+GCC_WARNS += -Wpointer-compare -Wredundant-decls -Wrestrict -Wreturn-type
+GCC_WARNS += -Wsequence-point -Wshadow -Wshift-overflow=2 -Wshift-count-negative
+GCC_WARNS += -Wshift-count-overflow -Wshift-negative-value -Wfloat-conversion
+GCC_WARNS += -Wsizeof-pointer-memaccess -Wsizeof-array-argument
+GCC_WARNS += -Wstack-protector -Wstrict-aliasing -Wstrict-overflow=5
+GCC_WARNS += -Wstringop-overflow=4 -Wsuggest-final-types -Wsuggest-final-methods
+GCC_WARNS += -Wmissing-format-attribute -Wswitch -Wswitch-bool -Wswitch-enum
+GCC_WARNS += -Wswitch-unreachable -Wsync-nand -Wtautological-compare
+GCC_WARNS += -Wtrampolines -Wtrigraphs -Wtype-limits -Wundef -Wuninitialized
+GCC_WARNS += -Wunknown-pragmas -Wunsafe-loop-optimizations
+GCC_WARNS += -Wunsuffixed-float-constants -Wunused -Wunused-function
+GCC_WARNS += -Wunused-label -Wunused-local-typedefs -Wunused-macros
+GCC_WARNS += -Wunused-value -Wunused-variable -Wunused-const-variable=2
+GCC_WARNS += -Wunused-but-set-parameter -Wunused-but-set-variable
+GCC_WARNS += -Wvariadic-macros -Wvector-operation-performance -Wvla
+GCC_WARNS += -Wvolatile-register-var -Wwrite-strings
+
+# "Issue a warning when HSAIL cannot be emitted for the compiled function or
+# OpenMP construct."
+# Uh-huh.
+GCC_WARNS += -Whsa
+
+# I don't mind too much increasing these.
+# Just make sure that you know what you're doing.
+GCC_WARNS += -Wlarger-than=1024 -Walloc-size-larger-than=4096
+GCC_WARNS += -Wframe-larger-than=1024 -Wstack-usage=1024
+
+# Can't use because of dependencies: -Waggregate-return
+# Want to add, but needs work: -Wconversion, -Wsign-compare, -Wsign-conversion
+# Seem to require other compiler features: -Wchkp, -Wstack-protector,
+#     -Wstrict-aliasing, -Wstrict-overflow
diff --git a/src/address.c b/src/address.c
index 63da1971..7e6b953d 100644
--- a/src/address.c
+++ b/src/address.c
@@ -65,7 +65,7 @@ prefix4_decode(IPAddress2_t *str, struct ipv4_prefix *result)
 	int len;
 
 	if (str->size > 4) {
-		return pr_err("IPv4 address has too many octets. (%u)",
+		return pr_err("IPv4 address has too many octets. (%zu)",
 		    str->size);
 	}
 	if (str->bits_unused < 0 || 7 < str->bits_unused) {
@@ -102,7 +102,7 @@ prefix6_decode(IPAddress2_t *str, struct ipv6_prefix *result)
 	int len;
 
 	if (str->size > 16) {
-		return pr_err("IPv6 address has too many octets. (%u)",
+		return pr_err("IPv6 address has too many octets. (%zu)",
 		    str->size);
 	}
 	if (str->bits_unused < 0 || 7 < str->bits_unused) {
@@ -115,7 +115,7 @@ prefix6_decode(IPAddress2_t *str, struct ipv6_prefix *result)
 	len = 8 * str->size - str->bits_unused;
 
 	if (len < 0 || 128 < len) {
-		return pr_err("IPv6 prefix length (%u) is out of bounds (0-128).",
+		return pr_err("IPv6 prefix length (%d) is out of bounds (0-128).",
 		    len);
 	}
 
diff --git a/src/asn1/decode.c b/src/asn1/decode.c
index 087848cf..2800c52d 100644
--- a/src/asn1/decode.c
+++ b/src/asn1/decode.c
@@ -35,7 +35,7 @@ asn1_decode(const void *buffer, size_t buffer_size,
 		/* Must free partial object according to API contracts. */
 		ASN_STRUCT_FREE(*descriptor, *result);
 		/* TODO if rval.code == RC_WMORE (1), more work is needed */
-		return pr_err("Error decoding ASN.1 object: %d", rval.code);
+		return pr_err("Error decoding ASN.1 object: %u", rval.code);
 	}
 
 	error = validate(descriptor, *result);
diff --git a/src/asn1/signed_data.c b/src/asn1/signed_data.c
index 5375d0ba..1a0517b8 100644
--- a/src/asn1/signed_data.c
+++ b/src/asn1/signed_data.c
@@ -18,7 +18,7 @@ static const OID oid_sta = OID_SIGNING_TIME_ATTR;
 static const OID oid_bsta = OID_BINARY_SIGNING_TIME_ATTR;
 
 static int
-is_digest_algorithm(AlgorithmIdentifier_t *id, char *what)
+is_digest_algorithm(AlgorithmIdentifier_t *id, char const *what)
 {
 	bool is_hash;
 	int error;
@@ -182,8 +182,7 @@ validate_signed_attrs(struct SignerInfo *sinfo, EncapsulatedContentInfo_t *eci)
 		attrs = &attr->attrValues;
 
 		if (attrs->list.count != 1) {
-			return pr_err(0,
-			    "signedAttrs's attribute set size (%d) is different than 1.",
+			return pr_err("signedAttrs's attribute set size (%d) is different than 1",
 			    attr->attrValues.list.count);
 		}
 		if (attrs->list.array == NULL || attrs->list.array[0] == NULL)
diff --git a/src/common.c b/src/common.c
index a35c6743..ab2be7ea 100644
--- a/src/common.c
+++ b/src/common.c
@@ -69,13 +69,15 @@ struct rfc5280_names {
 };
 
 static int
-get_names(X509_NAME *name, char *what, struct rfc5280_names *result)
+get_names(X509_NAME *name, char const *what, struct rfc5280_names *result)
 {
 	int error;
 
 	error = x509_name_decode(name, NID_commonName, &result->commonName);
-	if (error == -ESRCH)
-		return pr_err("The '%s' name lacks a commonName attribute.");
+	if (error == -ESRCH) {
+		return pr_err("The '%s' name lacks a commonName attribute.",
+		    what);
+	}
 	if (error)
 		return error;
 
diff --git a/src/debug.c b/src/debug.c
index 53681054..f1e325ee 100644
--- a/src/debug.c
+++ b/src/debug.c
@@ -15,12 +15,13 @@
  */
 void print_stack_trace(void)
 {
-	void *array[256];
+#define STACK_SIZE 64
+	void *array[STACK_SIZE];
 	size_t size;
 	char **strings;
 	size_t i;
 
-	size = backtrace(array, 256);
+	size = backtrace(array, STACK_SIZE);
 	strings = backtrace_symbols(array, size);
 
 	fprintf(stderr, "Stack trace:\n");
diff --git a/src/extension.c b/src/extension.c
index 18ea9454..ab7d53c7 100644
--- a/src/extension.c
+++ b/src/extension.c
@@ -9,13 +9,13 @@
 
 static struct extension_metadata IR2 = {
 	"Amended IP Resources",
-	/* TODO NID_ipAddrBlocksv2 */ -1,
+	-1,
 	true,
 };
 
 static struct extension_metadata AR2 = {
 	"Amended AS Resources",
-	/* TODO NID_autonomousSysIdsv2 */ -1,
+	-1,
 	true,
 };
 
diff --git a/src/extension.h b/src/extension.h
index 707c1991..74817ff6 100644
--- a/src/extension.h
+++ b/src/extension.h
@@ -5,7 +5,7 @@
 #include <openssl/x509.h>
 
 struct extension_metadata {
-	char *name;
+	char const *name;
 	int nid;
 	bool critical;
 };
@@ -13,11 +13,10 @@ struct extension_metadata {
 struct extension_handler {
 	struct extension_metadata const *meta;
 	bool mandatory;
+
 	int (*cb)(X509_EXTENSION *, void *);
 	void *arg;
 
-	void (*free)(void *);
-
 	/* For internal use */
 	bool found;
 };
diff --git a/src/log.h b/src/log.h
index fa002c17..43ead610 100644
--- a/src/log.h
+++ b/src/log.h
@@ -10,11 +10,22 @@
  *   to ease debugging.
  */
 
+#if __GNUC__
+#define CHECK_FORMAT(str, args) __attribute__((format(printf, str, args)))
+#else
+/*
+ * No idea how this looks in other compilers.
+ * It's safe to obviate since we're bound to see the warnings every time we use
+ * GCC anyway.
+ */
+#define CHECK_FORMAT(str, args) /* Nothing */
+#endif
+
 #ifdef DEBUG
 
-void pr_debug(const char *, ...);
-void pr_debug_add(const char *, ...);
-void pr_debug_rm(const char *, ...);
+void pr_debug(const char *, ...) CHECK_FORMAT(1, 2);
+void pr_debug_add(const char *, ...) CHECK_FORMAT(1, 2);
+void pr_debug_rm(const char *, ...) CHECK_FORMAT(1, 2);
 void pr_debug_prefix(void);
 
 #else
@@ -26,13 +37,12 @@ void pr_debug_prefix(void);
 
 #endif
 
-int pr_warn(const char *, ...);
-int pr_err(const char *, ...);
-int pr_errno(int, const char *, ...);
-int crypto_err(const char *, ...);
+int pr_warn(const char *, ...) CHECK_FORMAT(1, 2);
+int pr_err(const char *, ...) CHECK_FORMAT(1, 2);
+int pr_errno(int, const char *, ...) CHECK_FORMAT(2, 3);
+int crypto_err(const char *, ...) CHECK_FORMAT(1, 2);
 int pr_enomem(void);
-
-int pr_crit(const char *, ...);
+int pr_crit(const char *, ...) CHECK_FORMAT(1, 2);
 
 #define PR_DEBUG printf("%s:%d (%s())\n", __FILE__, __LINE__, __func__)
 #define PR_DEBUG_MSG(msg, ...) printf("%s:%d (%s()): " msg "\n", \
diff --git a/src/object/certificate.c b/src/object/certificate.c
index 34bd3afe..461457cb 100644
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -745,12 +745,12 @@ handle_ku(X509_EXTENSION *ext, unsigned char byte1)
 	memcpy(data, ku->data, ku->length);
 
 	if (ku->data[0] != byte1) {
-		error = pr_err("Illegal key usage flag string: %u%u%u%u%u%u%u%u%u",
-		    !!(ku->data[0] & 0x80), !!(ku->data[0] & 0x40),
-		    !!(ku->data[0] & 0x20), !!(ku->data[0] & 0x10),
-		    !!(ku->data[0] & 0x08), !!(ku->data[0] & 0x04),
-		    !!(ku->data[0] & 0x02), !!(ku->data[0] & 0x01),
-		    !!(ku->data[1] & 0x80));
+		error = pr_err("Illegal key usage flag string: %d%d%d%d%d%d%d%d%d",
+		    !!(ku->data[0] & 0x80u), !!(ku->data[0] & 0x40u),
+		    !!(ku->data[0] & 0x20u), !!(ku->data[0] & 0x10u),
+		    !!(ku->data[0] & 0x08u), !!(ku->data[0] & 0x04u),
+		    !!(ku->data[0] & 0x02u), !!(ku->data[0] & 0x01u),
+		    !!(ku->data[1] & 0x80u));
 		goto end;
 	}
 
@@ -781,14 +781,14 @@ handle_cdp(X509_EXTENSION *ext, void *arg)
 	GENERAL_NAME *name;
 	int i;
 	int error = 0;
-	char *error_msg;
+	char const *error_msg;
 
 	crldp = X509V3_EXT_d2i(ext);
 	if (crldp == NULL)
 		return cannot_decode(ext_cdp());
 
 	if (sk_DIST_POINT_num(crldp) != 1) {
-		error = pr_err("The %s extension has %u distribution points. (1 expected)",
+		error = pr_err("The %s extension has %d distribution points. (1 expected)",
 		    ext_cdp()->name, sk_DIST_POINT_num(crldp));
 		goto end;
 	}
@@ -877,7 +877,8 @@ end:
  *    are also allowed, and also supposed to be ignored.
  */
 static int
-handle_ad(char *ia_name, SIGNATURE_INFO_ACCESS *ia, char *ad_name, int ad_nid,
+handle_ad(char const *ia_name, SIGNATURE_INFO_ACCESS *ia,
+    char const *ad_name, int ad_nid,
     int (*cb)(struct rpki_uri *, void *), void *arg)
 {
 	ACCESS_DESCRIPTION *ad;
@@ -1013,7 +1014,7 @@ handle_cp(X509_EXTENSION *ext, void *arg)
 		return cannot_decode(ext_cp());
 
 	if (sk_POLICYINFO_num(cp) != 1) {
-		error = pr_err("The %s extension has %u policy information's. (1 expected)",
+		error = pr_err("The %s extension has %d policy information's. (1 expected)",
 		    ext_cp()->name, sk_POLICYINFO_num(cp));
 		goto end;
 	}
diff --git a/src/object/crl.c b/src/object/crl.c
index 8bd2caa7..059eaa14 100644
--- a/src/object/crl.c
+++ b/src/object/crl.c
@@ -117,10 +117,8 @@ crl_validate(X509_CRL *crl)
 	int error;
 
 	version = X509_CRL_get_version(crl);
-	if (version != 1) {
-		return pr_err("CRL version (0x%lx) is not 2 (0x%x).",
-		    version, 1);
-	}
+	if (version != 1)
+		return pr_err("CRL version (%ld) is not v2 (%d).", version, 1);
 
 	if (X509_CRL_get_signature_nid(crl) != rpki_signature_algorithm())
 		return pr_err("CRL's signature algorithm is not RSASSA-PKCS1-v1_5.");
diff --git a/src/resource.c b/src/resource.c
index dd3733b3..f58a0eb9 100644
--- a/src/resource.c
+++ b/src/resource.c
@@ -52,7 +52,7 @@ int
 get_addr_family(OCTET_STRING_t *octets)
 {
 	if (octets->size != 2) {
-		pr_err("Address family has %d octets. (2 expected.)",
+		pr_err("Address family has %zu octets. (2 expected.)",
 		    octets->size);
 		return -1;
 	}
@@ -349,7 +349,7 @@ add_aors(struct resources *resources, int family,
 			break;
 		case IPAddressOrRange_PR_NOTHING:
 			/* rfc3779#section-2.2.3.7 */
-			return pr_err("Unknown IPAddressOrRange type: %d",
+			return pr_err("Unknown IPAddressOrRange type: %u",
 			    aor->present);
 		}
 	}
@@ -377,7 +377,7 @@ resources_add_ip(struct resources *resources, struct IPAddressFamily *obj)
 	}
 
 	/* rfc3779#section-2.2.3.4 */
-	return pr_err("Unknown ipAddressChoice type: %d",
+	return pr_err("Unknown ipAddressChoice type: %u",
 	    obj->ipAddressChoice.present);
 }
 
@@ -495,7 +495,7 @@ add_asior(struct resources *resources, struct ASIdOrRange *obj)
 		return add_asn(resources, asn_min, asn_max, parent);
 	}
 
-	return pr_err("Unknown ASIdOrRange type: %d", obj->present);
+	return pr_err("Unknown ASIdOrRange type: %u", obj->present);
 }
 
 int
@@ -528,7 +528,7 @@ resources_add_asn(struct resources *resources, struct ASIdentifiers *ids)
 		break;
 	}
 
-	return pr_err("Unknown ASIdentifierChoice: %d", ids->asnum->present);
+	return pr_err("Unknown ASIdentifierChoice: %u", ids->asnum->present);
 }
 
 bool
diff --git a/src/sorted_array.c b/src/sorted_array.c
index 8ee83c4a..6c29b715 100644
--- a/src/sorted_array.c
+++ b/src/sorted_array.c
@@ -100,7 +100,7 @@ compare(struct sorted_array *sarray, void *new)
 		return -EINTERSECTION;
 	}
 
-	return pr_crit("Unknown comparison value: %d", cmp);
+	return pr_crit("Unknown comparison value: %u", cmp);
 }
 
 int
@@ -166,7 +166,7 @@ sarray_contains(struct sorted_array *sarray, void *elem)
 			break;
 		}
 
-		pr_crit("Unknown comparison value: %d", cmp);
+		pr_crit("Unknown comparison value: %u", cmp);
 		return false;
 	}
 
-- 
2.47.3