From 4f1fae3043f22d3a2a0c7fcd7d0244cd91b60bdf Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 15 Dec 2021 11:41:10 -0500 Subject: [PATCH] Fix the easy -Wformat-signedness errors. GCC has a warning that complains about even more type mismatches in printf. Some of these are a bit messy and will be fixed in separate CLs. This covers the easy ones. The .*s stuff is unfortunate, but printf has no size_t-clean string printer. ALPN protocol lengths are bound by uint8_t, so it doesn't really matter. The IPv6 printing one is obnoxious and arguably a false positive. It's really a C language flaw: all types smaller than int get converted to int when you do arithmetic. So something like this first doesn't overflow the shift because it computes over int, but then the result overall is stored as an int. uint8_t a, b; (a << 8) | b On the one hand, this fixes a "missing" cast to uint16_t before the shift. At the same time, the incorrect final type means passing it to %x, which expects unsigned int. The compiler has forgotten this value actually fits in uint16_t and flags a warning. Mitigate this by storing in a uint16_t first. The story doesn't quite end here. Arguments passed to variadic functions go through integer promotion[0], so the argument is still passed to snprintf as an int! But then va_arg allows for a signedness mismatch[1], provided the value is representable in both types. The combination means that %x, though actually paired with unsigned, also accept uint8_t and uint16_t, because those are guaranteed to promote to an int that meets [1]. GCC recognizes [1] applies here. (There's also PRI16x, but that's a bit tedious to use and, in glibc, is defined as plain "x" anyway.) [0] https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions [1] https://en.cppreference.com/w/c/variadic/va_arg Bug: 450 Change-Id: Ic1d41356755a18ab922956dd2e07b560470341f4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50765 Reviewed-by: Adam Langley Commit-Queue: Adam Langley --- crypto/dsa/dsa_test.cc | 2 +- crypto/evp/print.c | 4 ++-- crypto/fipsmodule/rand/urandom_test.cc | 2 +- crypto/x509v3/v3_alt.c | 8 +++++--- crypto/x509v3/v3_ncons.c | 3 ++- ssl/ssl_privkey.cc | 2 +- ssl/ssl_test.cc | 2 +- ssl/test/handshake_util.cc | 2 +- ssl/test/test_config.cc | 9 ++++----- tool/transport_common.cc | 6 +++--- 10 files changed, 21 insertions(+), 19 deletions(-) diff --git a/crypto/dsa/dsa_test.cc b/crypto/dsa/dsa_test.cc index 468213161..cee783d6c 100644 --- a/crypto/dsa/dsa_test.cc +++ b/crypto/dsa/dsa_test.cc @@ -243,7 +243,7 @@ static int TestGenerate(FILE *out) { fprintf(out, "%02X%02X%02X%02X ", seed[i], seed[i + 1], seed[i + 2], seed[i + 3]); } - fprintf(out, "\ncounter=%d h=%ld\n", counter, h); + fprintf(out, "\ncounter=%d h=%lu\n", counter, h); if (counter != 105) { fprintf(stderr, "counter should be 105\n"); diff --git a/crypto/evp/print.c b/crypto/evp/print.c index 3621d5f2c..0f4b65ea1 100644 --- a/crypto/evp/print.c +++ b/crypto/evp/print.c @@ -253,7 +253,7 @@ static int do_dsa_print(BIO *bp, const DSA *x, int off, int ptype) { if (priv_key) { if (!BIO_indent(bp, off, 128) || - BIO_printf(bp, "%s: (%d bit)\n", ktype, BN_num_bits(x->p)) <= 0) { + BIO_printf(bp, "%s: (%u bit)\n", ktype, BN_num_bits(x->p)) <= 0) { goto err; } } @@ -368,7 +368,7 @@ static int do_EC_KEY_print(BIO *bp, const EC_KEY *x, int off, int ktype) { } order = BN_new(); if (order == NULL || !EC_GROUP_get_order(group, order, NULL) || - BIO_printf(bp, "%s: (%d bit)\n", ecstr, BN_num_bits(order)) <= 0) { + BIO_printf(bp, "%s: (%u bit)\n", ecstr, BN_num_bits(order)) <= 0) { goto err; } diff --git a/crypto/fipsmodule/rand/urandom_test.cc b/crypto/fipsmodule/rand/urandom_test.cc index b9e900751..0a1d7539c 100644 --- a/crypto/fipsmodule/rand/urandom_test.cc +++ b/crypto/fipsmodule/rand/urandom_test.cc @@ -91,7 +91,7 @@ struct Event { switch (type) { case Syscall::kGetRandom: - snprintf(buf, sizeof(buf), "getrandom(_, %zu, %d)", length, flags); + snprintf(buf, sizeof(buf), "getrandom(_, %zu, %u)", length, flags); break; case Syscall::kOpen: diff --git a/crypto/x509v3/v3_alt.c b/crypto/x509v3/v3_alt.c index ce1c6e582..eb9c9757a 100644 --- a/crypto/x509v3/v3_alt.c +++ b/crypto/x509v3/v3_alt.c @@ -172,12 +172,13 @@ STACK_OF(CONF_VALUE) *i2v_GENERAL_NAME(X509V3_EXT_METHOD *method, case GEN_IPADD: p = gen->d.ip->data; if (gen->d.ip->length == 4) - BIO_snprintf(oline, sizeof oline, + BIO_snprintf(oline, sizeof(oline), "%d.%d.%d.%d", p[0], p[1], p[2], p[3]); else if (gen->d.ip->length == 16) { oline[0] = 0; for (i = 0; i < 8; i++) { - BIO_snprintf(htmp, sizeof htmp, "%X", p[0] << 8 | p[1]); + uint16_t v = ((uint16_t)p[0] << 8) | p[1]; + BIO_snprintf(htmp, sizeof(htmp), "%X", v); p += 2; OPENSSL_strlcat(oline, htmp, sizeof(oline)); if (i != 7) @@ -246,7 +247,8 @@ int GENERAL_NAME_print(BIO *out, GENERAL_NAME *gen) else if (gen->d.ip->length == 16) { BIO_printf(out, "IP Address"); for (i = 0; i < 8; i++) { - BIO_printf(out, ":%X", p[0] << 8 | p[1]); + uint16_t v = ((uint16_t)p[0] << 8) | p[1]; + BIO_printf(out, ":%X", v); p += 2; } BIO_puts(out, "\n"); diff --git a/crypto/x509v3/v3_ncons.c b/crypto/x509v3/v3_ncons.c index 739a59ec9..f58873e65 100644 --- a/crypto/x509v3/v3_ncons.c +++ b/crypto/x509v3/v3_ncons.c @@ -203,7 +203,8 @@ static int print_nc_ipadd(BIO *bp, ASN1_OCTET_STRING *ip) p[0], p[1], p[2], p[3], p[4], p[5], p[6], p[7]); } else if (len == 32) { for (i = 0; i < 16; i++) { - BIO_printf(bp, "%X", p[0] << 8 | p[1]); + uint16_t v = ((uint16_t)p[0] << 8) | p[1]; + BIO_printf(bp, "%X", v); p += 2; if (i == 7) BIO_puts(bp, "/"); diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc index 8462ebf00..41a13a24a 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -663,7 +663,7 @@ static bool parse_sigalgs_list(Array *out, const char *str) { // Note that the loop runs to len+1, i.e. it'll process the terminating NUL. for (size_t offset = 0; offset < len+1; offset++) { - const char c = str[offset]; + const unsigned char c = str[offset]; switch (c) { case '+': diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index d0bd680e1..328608189 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -4694,7 +4694,7 @@ std::string TicketAEADMethodParamToString( } } char retry_count[256]; - snprintf(retry_count, sizeof(retry_count), "%d", std::get<1>(params.param)); + snprintf(retry_count, sizeof(retry_count), "%u", std::get<1>(params.param)); ret += "_"; ret += retry_count; ret += "Retries_"; diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc index b9998317b..6516547fa 100644 --- a/ssl/test/handshake_util.cc +++ b/ssl/test/handshake_util.cc @@ -255,7 +255,7 @@ static bool Proxy(BIO *socket, bool async, int control, int rfd, int wfd) { return false; } if (bytes_written != bytes_read) { - fprintf(stderr, "short write (%zu of %d bytes)\n", bytes_written, + fprintf(stderr, "short write (%zd of %d bytes)\n", bytes_written, bytes_read); return false; } diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 7d1cefa16..16cb97198 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -1047,17 +1047,16 @@ static bool CheckCertificateRequest(SSL *ssl) { const size_t num_received = sk_X509_NAME_num(received); if (num_received != num_expected) { - fprintf(stderr, "expected %u names in CertificateRequest but got %u.\n", - static_cast(num_expected), - static_cast(num_received)); + fprintf(stderr, "expected %zu names in CertificateRequest but got %zu.\n", + num_expected, num_received); return false; } for (size_t i = 0; i < num_received; i++) { if (X509_NAME_cmp(sk_X509_NAME_value(received, i), sk_X509_NAME_value(expected.get(), i)) != 0) { - fprintf(stderr, "names in CertificateRequest differ at index #%d.\n", - static_cast(i)); + fprintf(stderr, "names in CertificateRequest differ at index #%zu.\n", + i); return false; } } diff --git a/tool/transport_common.cc b/tool/transport_common.cc index cba3c7b9c..991c808a2 100644 --- a/tool/transport_common.cc +++ b/tool/transport_common.cc @@ -306,13 +306,13 @@ void PrintConnectionInfo(BIO *bio, const SSL *ssl) { const uint8_t *next_proto; unsigned next_proto_len; SSL_get0_next_proto_negotiated(ssl, &next_proto, &next_proto_len); - BIO_printf(bio, " Next protocol negotiated: %.*s\n", next_proto_len, - next_proto); + BIO_printf(bio, " Next protocol negotiated: %.*s\n", + static_cast(next_proto_len), next_proto); const uint8_t *alpn; unsigned alpn_len; SSL_get0_alpn_selected(ssl, &alpn, &alpn_len); - BIO_printf(bio, " ALPN protocol: %.*s\n", alpn_len, alpn); + BIO_printf(bio, " ALPN protocol: %.*s\n", static_cast(alpn_len), alpn); const char *host_name = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); if (host_name != nullptr && SSL_is_server(ssl)) {