From 1361f693e7ad15513aaccb833e1bffcd54e41191 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= Date: Tue, 17 Sep 2024 15:36:52 +0200 Subject: [PATCH 1/2] Fix a sign propagation in calculating transaction size statistics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch fixes bugs in sign propagation disovered with -Wsign-conversion compilar flag and related integer overflows in dnf5::print_transaction_size_stats(), like this one: dnf5/main.cpp: In function ‘void dnf5::print_transaction_size_stats(Context&)’: dnf5/main.cpp:785:29: error: conversion to ‘long long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Werror=sign-conversion] 785 | in_pkgs_size += pkg_size; | ^~~~~~~~ There were more ways of fixing, but I choose this one: Use unsigned integers because an integer overflow for signed types is undefined in C++. I choose unsigned long long int to follow what get_download_size() and get_install_size() returns. Otherwise, we would have to add another check whether the value fits into e.g. uint64_t. Unless we chose uintmax_t. Explicitly type-cast counters, with a prior range check, on passing them to libdnf5::cli::utils::units::to_size(). Otherwise, we would have to add to_size() for unsigned integers to the public API. (Actuallly to_size() should have been for unsigned integeres from the very beginning. Package sizes cannot be negative.) Not printing the transaction size statistics if an irrepresentible value occurs. Adding more branches to the already complicated code for the very improbably corner cases does not make much sense. (I tried also a different implemeation with keeping int64_t as the counters, but the code was longer and uglier.) Related: https://github.com/rpm-software-management/dnf5/pull/1701 --- dnf5/main.cpp | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/dnf5/main.cpp b/dnf5/main.cpp index 87c61822c..a709afc39 100644 --- a/dnf5/main.cpp +++ b/dnf5/main.cpp @@ -85,6 +85,7 @@ along with libdnf. If not, see . #include #include #include +#include constexpr const char * DNF5_LOGGER_FILENAME = "dnf5.log"; @@ -773,29 +774,38 @@ static void print_versions(Context & context) { } static void print_transaction_size_stats(Context & context) { - int64_t in_pkgs_size{0}; - int64_t download_pkgs_size{0}; - int64_t install_size{0}; - int64_t remove_size{0}; + unsigned long long int in_pkgs_size{0}; + unsigned long long int download_pkgs_size{0}; + unsigned long long int install_size{0}; + unsigned long long int remove_size{0}; for (const auto & trans_pkg : context.get_transaction()->get_transaction_packages()) { const auto pkg = trans_pkg.get_package(); if (transaction_item_action_is_inbound(trans_pkg.get_action())) { const auto pkg_size = pkg.get_download_size(); in_pkgs_size += pkg_size; + if (in_pkgs_size < pkg_size) + return; if (!pkg.is_available_locally()) { download_pkgs_size += pkg_size; + if (download_pkgs_size < pkg_size) + return; } install_size += pkg.get_install_size(); + if (install_size < pkg.get_install_size()) + return; } else if (transaction_item_action_is_outbound(trans_pkg.get_action())) { remove_size += pkg.get_install_size(); + if (remove_size < pkg.get_install_size()) + return; } } - if (in_pkgs_size != 0) { - const auto [in_pkgs_size_value, in_pkgs_size_unit] = libdnf5::cli::utils::units::to_size(in_pkgs_size); + if (in_pkgs_size != 0 && std::in_range(in_pkgs_size) && std::in_range(download_pkgs_size)) { + const auto [in_pkgs_size_value, in_pkgs_size_unit] = + libdnf5::cli::utils::units::to_size(static_cast(in_pkgs_size)); const auto [dwnl_pkgs_size_value, dwnl_pkgs_size_unit] = - libdnf5::cli::utils::units::to_size(download_pkgs_size); + libdnf5::cli::utils::units::to_size(static_cast(download_pkgs_size)); context.print_info(libdnf5::utils::sformat( _("Total size of inbound packages is {:.0f} {:s}. Need to download {:.0f} {:s}."), in_pkgs_size_value, @@ -804,9 +814,14 @@ static void print_transaction_size_stats(Context & context) { dwnl_pkgs_size_unit)); } - const auto [install_size_value, install_size_unit] = libdnf5::cli::utils::units::to_size(install_size); - const auto [remove_size_value, remove_size_unit] = libdnf5::cli::utils::units::to_size(remove_size); - const auto size_diff = install_size - remove_size; + if (!std::in_range(install_size) || !std::in_range(remove_size) || + !std::in_range(install_size - remove_size)) + return; + const auto [install_size_value, install_size_unit] = + libdnf5::cli::utils::units::to_size(static_cast(install_size)); + const auto [remove_size_value, remove_size_unit] = + libdnf5::cli::utils::units::to_size(static_cast(remove_size)); + const int64_t size_diff = static_cast(install_size - remove_size); const auto [size_diff_value, size_diff_unit] = libdnf5::cli::utils::units::to_size(std::abs(size_diff)); if (size_diff >= 0) { context.print_info(libdnf5::utils::sformat( From a6d9b7f660739093107f20c6e97b4076963f466f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= Date: Tue, 17 Sep 2024 12:58:50 +0200 Subject: [PATCH 2/2] build: Add -Wsign-conversion to CXXFLAGS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -Wsign-conversion is by default enabled in Clang with -Wconversion, but disabled in GCC. Unintented type coercion can misinterpret the sign as in: libdnf5-cli/output/transaction_table.cpp:181:97: error: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Werror=sign-conversion] 181 | P_(" Skipping: {:4} package\n", " Skipping: {:4} packages\n", skips), skips) | ^~~~~ This would be missed when compiling with GCC, but causing a build failure with Clang. This patch brings GCC and Clang compilers in line, both to enable -Wsign-conversion. However, it disables the warning for SWIG-generated bindings as the generated code violates the warning many times, like here for Perl: bindings/perl5/libdnf5/CMakeFiles/perl5_advisory.dir/advisoryPERL_wrap.cxx:700:36: error: conversion to ‘long unsigned int’ from ‘long int’ may change the sign of the result [-Werror=sign-conversion] 700 | if (strlen(name) + 1 > (bsz - (r - buff))) return 0; | ~~~^~~~~~~ Related: https://github.com/rpm-software-management/dnf5/pull/1701 --- CMakeLists.txt | 2 +- bindings/CMakeLists.txt | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1621a8384..5cd3931fd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -83,7 +83,7 @@ add_compile_options("-fmacro-prefix-map=${PROJECT_SOURCE_DIR}/=") # warnings add_compile_options(-Wall -Wextra -Werror) -add_compile_options(-Wcast-align -Wformat-nonliteral -Wmissing-format-attribute -Wredundant-decls -Wsign-compare -Wtype-limits -Wuninitialized -Wwrite-strings) +add_compile_options(-Wcast-align -Wformat-nonliteral -Wmissing-format-attribute -Wredundant-decls -Wsign-compare -Wsign-conversion -Wtype-limits -Wuninitialized -Wwrite-strings) add_compile_options(-Werror=unused-result -Wodr) # not sure about the conversion warnings being errors; review later diff --git a/bindings/CMakeLists.txt b/bindings/CMakeLists.txt index d97ad2405..8593e8bcd 100644 --- a/bindings/CMakeLists.txt +++ b/bindings/CMakeLists.txt @@ -29,6 +29,7 @@ set(SWIG_COMPILE_OPTIONS -Wno-missing-declarations -Wno-missing-field-initializers -Wno-sign-compare + -Wno-sign-conversion -Wno-sometimes-uninitialized -Wno-strict-aliasing -Wno-unused-function