From 153a8ae657a0d2a03f43db076bdadfb4e7dff41e Mon Sep 17 00:00:00 2001

From: Aleš Matěj <amatej@redhat.com>

Date: Mon, 21 Jun 2021 12:20:54 +0200

Subject: [PATCH] Fix some covscan warnings

- missing return value checks

- remove useless/unused code

- set fp to NULL to prevent double free false positive

- checksum function in utils.cpp doesn't have a declaration and

  is used only in utils.cpp -> make it static

Some of these were also compiler warnings

---

 libdnf/dnf-repo.cpp                      | 15 ++++++++++-----

 libdnf/dnf-transaction.cpp               |  2 --

 libdnf/module/ModulePackageContainer.cpp |  2 --

 libdnf/repo/Repo.cpp                     | 28 +++++++++++++++++++++++-----

 libdnf/utils/filesystem.cpp              |  9 ++++++++-

 libdnf/utils/utils.cpp                   |  2 +-

 tests/hawkey/test_iutil.cpp              |  2 ++

 7 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/libdnf/dnf-repo.cpp b/libdnf/dnf-repo.cpp

index 710045f..cc6093e 100644

--- a/libdnf/dnf-repo.cpp

+++ b/libdnf/dnf-repo.cpp

@@ -1966,9 +1966,12 @@ out:

     g_free(updatedata.last_mirror_failure_message);

     g_free(updatedata.last_mirror_url);

     dnf_state_release_locks(state);

-    lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSCB, NULL);

-    lr_handle_setopt(priv->repo_handle, NULL, LRO_HMFCB, NULL);

-    lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSDATA, 0xdeadbeef);

+    if (!lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSCB, NULL))

+            g_debug("Failed to reset LRO_PROGRESSCB to NULL");

+    if (!lr_handle_setopt(priv->repo_handle, NULL, LRO_HMFCB, NULL))

+            g_debug("Failed to reset LRO_HMFCB to NULL");

+    if (!lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSDATA, 0xdeadbeef))

+            g_debug("Failed to set LRO_PROGRESSDATA to 0xdeadbeef");

     return ret;

 } CATCH_TO_GERROR(FALSE)

@@ -2296,8 +2299,10 @@ dnf_repo_download_packages(DnfRepo *repo,

     ret = TRUE;

 out:

-    lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSCB, NULL);

-    lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSDATA, 0xdeadbeef);

+    if (!lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSCB, NULL))

+            g_debug("Failed to reset LRO_PROGRESSCB to NULL");

+    if (!lr_handle_setopt(priv->repo_handle, NULL, LRO_PROGRESSDATA, 0xdeadbeef))

+            g_debug("Failed to set LRO_PROGRESSDATA to 0xdeadbeef");

     g_free(global_data.last_mirror_failure_message);

     g_free(global_data.last_mirror_url);

     g_slist_free_full(package_targets, (GDestroyNotify)lr_packagetarget_free);

diff --git a/libdnf/dnf-transaction.cpp b/libdnf/dnf-transaction.cpp

index 7c9a3a9..e096658 100644

--- a/libdnf/dnf-transaction.cpp

+++ b/libdnf/dnf-transaction.cpp

@@ -1523,8 +1523,6 @@ dnf_transaction_commit(DnfTransaction *transaction, HyGoal goal, DnfState *state

     /* this section done */

     ret = dnf_state_done(state, error);

-    if (!ret)

-        goto out;

 out:

     dnf_transaction_reset(transaction);

     dnf_state_release_locks(state);

diff --git a/libdnf/module/ModulePackageContainer.cpp b/libdnf/module/ModulePackageContainer.cpp

index a20f8df..dc551a9 100644

--- a/libdnf/module/ModulePackageContainer.cpp

+++ b/libdnf/module/ModulePackageContainer.cpp

@@ -1753,8 +1753,6 @@ void ModulePackageContainer::updateFailSafeData()

     if (pImpl->activatedModules) {

         std::vector<ModulePackage *> latest = pImpl->getLatestActiveEnabledModules();

-        auto begin = fileNames.begin();

-        auto end = fileNames.end();

         if (g_mkdir_with_parents(pImpl->persistDir.c_str(), 0755) == -1) {

             const char * errTxt = strerror(errno);

             auto logger(Log::getLogger());

diff --git a/libdnf/repo/Repo.cpp b/libdnf/repo/Repo.cpp

index e3a574f..e1d5529 100644

--- a/libdnf/repo/Repo.cpp

+++ b/libdnf/repo/Repo.cpp

@@ -704,7 +704,11 @@ static std::vector<Key> rawkey2infos(int fd) {

     // set GPG home dir

     char tmpdir[] = "/tmp/tmpdir.XXXXXX";

-    mkdtemp(tmpdir);

+    if (!mkdtemp(tmpdir)) {

+        const char * errTxt = strerror(errno);

+        throw RepoError(tfm::format(_("Cannot create repo temporary directory \"%s\": %s"),

+                                      tmpdir, errTxt));

+    }

     Finalizer tmpDirRemover([&tmpdir](){

         dnf_remove_recursive(tmpdir, NULL);

     });

@@ -894,8 +898,14 @@ void Repo::Impl::importRepoKeys()

             }

             struct stat sb;

-            if (stat(gpgDir.c_str(), &sb) != 0 || !S_ISDIR(sb.st_mode))

-                mkdir(gpgDir.c_str(), 0777);

+            if (stat(gpgDir.c_str(), &sb) != 0 || !S_ISDIR(sb.st_mode)) {

+                int res = mkdir(gpgDir.c_str(), 0777);

+                if (res != 0 && errno != EEXIST) {

+                    auto msg = tfm::format(_("Failed to create directory \"%s\": %d - %s"),

+                                           gpgDir, errno, strerror(errno));

+                    throw RepoError(msg);

+                }

+            }

             gpgme_ctx_t ctx;

             gpgme_new(&ctx;;

@@ -1147,7 +1157,11 @@ bool Repo::Impl::isMetalinkInSync()

 {

     auto logger(Log::getLogger());

     char tmpdir[] = "/tmp/tmpdir.XXXXXX";

-    mkdtemp(tmpdir);

+    if (!mkdtemp(tmpdir)) {

+        const char * errTxt = strerror(errno);

+        throw RepoError(tfm::format(_("Cannot create repo temporary directory \"%s\": %s"),

+                                      tmpdir, errTxt));

+    }

     Finalizer tmpDirRemover([&tmpdir](){

         dnf_remove_recursive(tmpdir, NULL);

     });

@@ -1217,7 +1231,11 @@ bool Repo::Impl::isRepomdInSync()

     auto logger(Log::getLogger());

     LrYumRepo *yum_repo;

     char tmpdir[] = "/tmp/tmpdir.XXXXXX";

-    mkdtemp(tmpdir);

+    if (!mkdtemp(tmpdir)) {

+        const char * errTxt = strerror(errno);

+        throw RepoError(tfm::format(_("Cannot create repo temporary directory \"%s\": %s"),

+                                      tmpdir, errTxt));

+    }

     Finalizer tmpDirRemover([&tmpdir](){

         dnf_remove_recursive(tmpdir, NULL);

     });

diff --git a/libdnf/utils/filesystem.cpp b/libdnf/utils/filesystem.cpp

index c3abd3c..89a9478 100644

--- a/libdnf/utils/filesystem.cpp

+++ b/libdnf/utils/filesystem.cpp

@@ -24,6 +24,8 @@

 #include <cerrno>

 #include <cstring>

+#include "bgettext/bgettext-lib.h"

+#include "tinyformat/tinyformat.hpp"

 #include "filesystem.hpp"

 #include "../error.hpp"

@@ -72,7 +74,12 @@ makeDirPath(std::string filePath)

         previous = position;

         // create directory if necessary

         if (!pathExists(directory.c_str())) {

-            mkdir(directory.c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);

+            int res = mkdir(directory.c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);

+            if (res != 0 && errno != EEXIST) {

+                auto msg = tfm::format(_("Failed to create directory \"%s\": %d - %s"),

+                                       directory, errno, strerror(errno));

+                throw Error(msg);

+            }

         }

     }

 }

diff --git a/libdnf/utils/utils.cpp b/libdnf/utils/utils.cpp

index 450718d..15f5275 100644

--- a/libdnf/utils/utils.cpp

+++ b/libdnf/utils/utils.cpp

@@ -301,7 +301,7 @@ void decompress(const char * inPath, const char * outPath, mode_t outMode, const

     fclose(inFile);

 }

-void checksum(const char * type, const char * inPath, const char * checksum_valid, bool * valid_out, gchar ** calculated_out)

+static void checksum(const char * type, const char * inPath, const char * checksum_valid, bool * valid_out, gchar ** calculated_out)

 {

     GError * errP{nullptr};

     gboolean valid;

diff --git a/tests/hawkey/test_iutil.cpp b/tests/hawkey/test_iutil.cpp

index 8d00cc9..6dafbdc 100644

--- a/tests/hawkey/test_iutil.cpp

+++ b/tests/hawkey/test_iutil.cpp

@@ -79,11 +79,13 @@ START_TEST(test_checksum)

     /* the taken checksum are not zeros anymore */

     fail_if(checksum_cmp(cs1, cs2) == 0);

     fail_if(checksum_cmp(cs1_sum, cs2_sum) == 0);

+    fp = NULL;

     /* append something */

     fail_if((fp = fopen(new_file, "a")) == NULL);

     fail_unless(fwrite("X", 1, 1, fp) == 1);

     fclose(fp);

+    fp = NULL;

     /* take the second checksums */

     fail_if((fp = fopen(new_file, "r")) == NULL);

--

libgit2 1.0.1