From d30404868f2bf7725f5dd36245e1149a09349bfd Mon Sep 17 00:00:00 2001 From: Austin Schuh Date: Mon, 27 Sep 2021 04:54:05 -0700 Subject: [PATCH] ARM and -Wextra fixups (#19141) * Make generated code compile with -Wunused-parameters -Wunused-parameters is turned on by -Werror -Wextra. gRPC code generation creates header files with unused parameters. So let's modify the generator to not generate code which will make -Wunused-parameters unhappy. * Fix unsigned vs signed comparisons and 32 bit string formats Fix unsigned vs signed comparison warnings. For 64 bit numbers printed in gRPC, the string formats assume that you are running on a 64 bit machine. Use inttypes.h to make it portable. Also, use size_t format strings for the same reason. * Fix unaligned memory access cost_entry_ptr has no alignment guarentees that ubsan can find. So it fails the test with an alignment problem. Use memcopy to read the data from the pointer to fix this. * Fix undefined behavior with memcpy and memcmp Passing in a 0 length piece of data and a null pointer is undefined behavior. If the length is 0, don't pass it in. This fixes ubsan failures. * Clang-format * Automated change: Fix sanity tests --- .../server_load_reporting_filter.cc | 9 +++-- src/core/tsi/alts/crypt/aes_gcm.cc | 4 ++- .../load_reporter/get_cpu_stats_linux.cc | 5 ++- test/core/tsi/alts/crypt/aes_gcm_test.cc | 36 +++++++++++++------ test/core/tsi/alts/crypt/gsec_test_util.cc | 4 ++- test/cpp/qps/client.h | 4 +-- .../load_reporter/load_reporter_test.cc | 2 +- .../address_sorting/address_sorting_posix.c | 1 + 8 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc b/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc index 2427bc18699..9be2f739a28 100644 --- a/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc +++ b/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc @@ -200,8 +200,9 @@ void ServerLoadReportingCallData::StoreClientIpAndLrToken(const char* lr_token, if (lr_token_len != 0) { strncpy(cur_pos, lr_token, lr_token_len); } - GPR_ASSERT(cur_pos + lr_token_len - client_ip_and_lr_token_ == - long(client_ip_and_lr_token_len_)); + GPR_ASSERT( + static_cast(cur_pos + lr_token_len - client_ip_and_lr_token_) == + client_ip_and_lr_token_len_); } grpc_filtered_mdelem ServerLoadReportingCallData::RecvInitialMetadataFilter( @@ -292,7 +293,9 @@ grpc_filtered_mdelem ServerLoadReportingCallData::SendTrailingMetadataFilter( } const double* cost_entry_ptr = reinterpret_cast(GRPC_SLICE_START_PTR(value)); - double cost_value = *cost_entry_ptr++; + double cost_value; + memcpy(&cost_value, cost_entry_ptr, sizeof(double)); + cost_entry_ptr++; const char* cost_name = reinterpret_cast(cost_entry_ptr); const size_t cost_name_len = cost_entry_size - sizeof(double); opencensus::stats::Record( diff --git a/src/core/tsi/alts/crypt/aes_gcm.cc b/src/core/tsi/alts/crypt/aes_gcm.cc index b761224cd93..4eaaee2898b 100644 --- a/src/core/tsi/alts/crypt/aes_gcm.cc +++ b/src/core/tsi/alts/crypt/aes_gcm.cc @@ -559,7 +559,9 @@ static grpc_status_code gsec_aes_gcm_aead_crypter_decrypt_iovec( if (!EVP_DecryptFinal_ex(aes_gcm_crypter->ctx, nullptr, &bytes_written_temp)) { aes_gcm_format_errors("Checking tag failed.", error_details); - memset(plaintext_vec.iov_base, 0x00, plaintext_vec.iov_len); + if (plaintext_vec.iov_base != nullptr) { + memset(plaintext_vec.iov_base, 0x00, plaintext_vec.iov_len); + } return GRPC_STATUS_FAILED_PRECONDITION; } if (bytes_written_temp != 0) { diff --git a/src/cpp/server/load_reporter/get_cpu_stats_linux.cc b/src/cpp/server/load_reporter/get_cpu_stats_linux.cc index 561d4f50482..f778b137855 100644 --- a/src/cpp/server/load_reporter/get_cpu_stats_linux.cc +++ b/src/cpp/server/load_reporter/get_cpu_stats_linux.cc @@ -20,6 +20,8 @@ #ifdef GPR_LINUX +#include + #include #include "src/cpp/server/load_reporter/get_cpu_stats.h" @@ -32,7 +34,8 @@ std::pair GetCpuStatsImpl() { FILE* fp; fp = fopen("/proc/stat", "r"); uint64_t user, nice, system, idle; - if (fscanf(fp, "cpu %lu %lu %lu %lu", &user, &nice, &system, &idle) != 4) { + if (fscanf(fp, "cpu %" PRIu64 " %" PRIu64 " %" PRIu64 " %" PRIu64, &user, + &nice, &system, &idle) != 4) { // Something bad happened with the information, so assume it's all invalid user = nice = system = idle = 0; } diff --git a/test/core/tsi/alts/crypt/aes_gcm_test.cc b/test/core/tsi/alts/crypt/aes_gcm_test.cc index c7c86fb3ae3..db475376350 100644 --- a/test/core/tsi/alts/crypt/aes_gcm_test.cc +++ b/test/core/tsi/alts/crypt/aes_gcm_test.cc @@ -120,7 +120,9 @@ static void gsec_test_random_encrypt_decrypt(gsec_aead_crypter* crypter, GPR_ASSERT(status == GRPC_STATUS_OK); GPR_ASSERT(message_length == plaintext_bytes_written); - GPR_ASSERT(memcmp(message, plaintext, message_length) == 0); + if (message_length != 0) { + GPR_ASSERT(memcmp(message, plaintext, message_length) == 0); + } /** * The returned plaintext will be zeroed if there was an authentication error. @@ -142,7 +144,9 @@ static void gsec_test_random_encrypt_decrypt(gsec_aead_crypter* crypter, status, GRPC_STATUS_FAILED_PRECONDITION, "Checking tag failed.", error_message)); GPR_ASSERT(plaintext_bytes_written == 0); - GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + if (plaintext_length != 0) { + GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + } gpr_free(corrupt_nonce); gpr_free(error_message); } @@ -162,7 +166,9 @@ static void gsec_test_random_encrypt_decrypt(gsec_aead_crypter* crypter, status, GRPC_STATUS_FAILED_PRECONDITION, error_message, "Checking tag failed")); GPR_ASSERT(plaintext_bytes_written == 0); - GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + if (plaintext_length != 0) { + GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + } gpr_free(error_message); gpr_free(corrupt_ciphertext_and_tag); @@ -176,7 +182,9 @@ static void gsec_test_random_encrypt_decrypt(gsec_aead_crypter* crypter, corrupt_ciphertext_and_tag, ciphertext_bytes_written, plaintext, plaintext_length, &plaintext_bytes_written, &error_message); GPR_ASSERT(plaintext_bytes_written == 0); - GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + if (plaintext_length != 0) { + GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + } GPR_ASSERT(gsec_test_expect_compare_code_and_substr( status, GRPC_STATUS_FAILED_PRECONDITION, error_message, "Checking tag failed")); @@ -198,7 +206,9 @@ static void gsec_test_random_encrypt_decrypt(gsec_aead_crypter* crypter, status, GRPC_STATUS_FAILED_PRECONDITION, error_message, "Checking tag failed")); GPR_ASSERT(plaintext_bytes_written == 0); - GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + if (plaintext_length != 0) { + GPR_ASSERT(memcmp(zero_message, plaintext, plaintext_length) == 0); + } gpr_free(error_message); gpr_free(corrupt_ciphertext_and_tag); } @@ -292,7 +302,9 @@ static void gsec_test_multiple_random_encrypt_decrypt( &(plaintext_bytes_writtens[ind]), nullptr); GPR_ASSERT(status == GRPC_STATUS_OK); GPR_ASSERT(message_length == plaintext_bytes_writtens[ind]); - GPR_ASSERT(memcmp(messages[ind], plaintexts[ind], message_length) == 0); + if (message_length != 0) { + GPR_ASSERT(memcmp(messages[ind], plaintexts[ind], message_length) == 0); + } } /* Slice the plaintext and encrypt with iovecs */ @@ -358,8 +370,10 @@ static void gsec_test_multiple_random_encrypt_decrypt( decrypted_vec, &decrypted_length, &error_details), error_details); GPR_ASSERT(decrypted_vec.iov_len == message_length); - GPR_ASSERT(memcmp(decrypted_vec.iov_base, messages[ind], message_length) == - 0); + if (message_length != 0) { + GPR_ASSERT( + memcmp(decrypted_vec.iov_base, messages[ind], message_length) == 0); + } free(decrypted); free(aad_vecs); free(ciphertext_vecs); @@ -713,8 +727,10 @@ static void gsec_test_encrypt_decrypt_test_vector( test_vector->ciphertext_and_tag_length, plaintext_bytes, plaintext_length, &plaintext_bytes_written, nullptr); GPR_ASSERT(status == GRPC_STATUS_OK); - GPR_ASSERT(memcmp(test_vector->plaintext, plaintext_bytes, - plaintext_bytes_written) == 0); + if (plaintext_bytes_written != 0) { + GPR_ASSERT(memcmp(test_vector->plaintext, plaintext_bytes, + plaintext_bytes_written) == 0); + } gpr_free(ciphertext_and_tag_bytes); gpr_free(plaintext_bytes); diff --git a/test/core/tsi/alts/crypt/gsec_test_util.cc b/test/core/tsi/alts/crypt/gsec_test_util.cc index 89c887f9a27..29533fa80e6 100644 --- a/test/core/tsi/alts/crypt/gsec_test_util.cc +++ b/test/core/tsi/alts/crypt/gsec_test_util.cc @@ -49,7 +49,9 @@ uint32_t gsec_test_bias_random_uint32(uint32_t max_length) { void gsec_test_copy(const uint8_t* src, uint8_t** des, size_t source_len) { if (src != nullptr && des != nullptr) { *des = static_cast(gpr_malloc(source_len)); - memcpy(*des, src, source_len); + if (*des != nullptr) { + memcpy(*des, src, source_len); + } } else { fprintf(stderr, "Either src or des buffer is nullptr in gsec_test_copy()."); abort(); diff --git a/test/cpp/qps/client.h b/test/cpp/qps/client.h index 9d2c7236649..61914e93da9 100644 --- a/test/cpp/qps/client.h +++ b/test/cpp/qps/client.h @@ -191,8 +191,8 @@ class Client { if (median_latency_collection_interval_seconds_ > 0) { std::vector medians_per_interval = threads_[0]->GetMedianPerIntervalList(); - gpr_log(GPR_INFO, "Num threads: %ld", threads_.size()); - gpr_log(GPR_INFO, "Number of medians: %ld", medians_per_interval.size()); + gpr_log(GPR_INFO, "Num threads: %zu", threads_.size()); + gpr_log(GPR_INFO, "Number of medians: %zu", medians_per_interval.size()); for (size_t j = 0; j < medians_per_interval.size(); j++) { gpr_log(GPR_INFO, "%f", medians_per_interval[j]); } diff --git a/test/cpp/server/load_reporter/load_reporter_test.cc b/test/cpp/server/load_reporter/load_reporter_test.cc index 9d84b045d83..480935fc056 100644 --- a/test/cpp/server/load_reporter/load_reporter_test.cc +++ b/test/cpp/server/load_reporter/load_reporter_test.cc @@ -176,7 +176,7 @@ class LbFeedbackTest : public LoadReporterTest { ASSERT_THAT(static_cast(lb_feedback.errors_per_second()), DoubleNear(expected_eps, expected_eps * 0.3)); gpr_log(GPR_INFO, - "Verified LB feedback matches the samples of index [%lu, %lu).", + "Verified LB feedback matches the samples of index [%zu, %zu).", start, start + count); } diff --git a/third_party/address_sorting/address_sorting_posix.c b/third_party/address_sorting/address_sorting_posix.c index d0dfe124691..dd969559be5 100644 --- a/third_party/address_sorting/address_sorting_posix.c +++ b/third_party/address_sorting/address_sorting_posix.c @@ -54,6 +54,7 @@ static bool posix_source_addr_factory_get_source_addr( address_sorting_source_addr_factory* factory, const address_sorting_address* dest_addr, address_sorting_address* source_addr) { + (void)factory; bool source_addr_exists = false; // Android sets SOCK_CLOEXEC. Don't set this here for portability. int s = socket(((struct sockaddr*)dest_addr)->sa_family, SOCK_DGRAM, 0);