Fix UB in LogSystem

Passing NULL to std::string constructor is an undefined behavior.

Check values returned from getenv() to avoid passing NULL to
std::string constructor. Use std::string when appropriate.

Update unit tests.

Change-Id: I56dd60f432c8e6e6033e9674601ced0b6432fc28

diff --git a/src/manager/common/log-setup.cpp b/src/manager/common/log-setup.cpp
index 5939907..0b5fbcc 100644 (file)
--- a/src/manager/common/log-setup.cpp
+++ b/src/manager/common/log-setup.cpp
@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2000 - 2015 Samsung Electronics Co., Ltd All Rights Reserved
+ *  Copyright (c) 2015 - 2020 Samsung Electronics Co., Ltd All Rights Reserved
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -114,7 +114,7 @@ void SetupClientLogSystem()
        const std::string level = parser.getLevel();
 
        if (!level.empty())
-               CKM::Singleton<CKM::Log::LogSystem>::Instance().SetLogLevel(level.c_str());
+               CKM::Singleton<CKM::Log::LogSystem>::Instance().SetLogLevel(level);
 
        logSystemReady = true;
 }

diff --git a/src/manager/dpl/log/include/dpl/log/log.h b/src/manager/dpl/log/include/dpl/log/log.h
index c169b9e..959fd00 100644 (file)
--- a/src/manager/dpl/log/include/dpl/log/log.h
+++ b/src/manager/dpl/log/include/dpl/log/log.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 Samsung Electronics Co., Ltd All Rights Reserved
+ * Copyright (c) 2011 - 2020 Samsung Electronics Co., Ltd All Rights Reserved
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
  *    you may not use this file except in compliance with the License.
@@ -83,7 +83,7 @@ public:
        /**
         * Sets log level (overwrites environment settings)
         */
-       void SetLogLevel(const char *level);
+       void SetLogLevel(const std::string& level);
 
 private:
        void RemoveProviders();

diff --git a/src/manager/dpl/log/src/log.cpp b/src/manager/dpl/log/src/log.cpp
index 8d86312..07d1e10 100644 (file)
--- a/src/manager/dpl/log/src/log.cpp
+++ b/src/manager/dpl/log/src/log.cpp
@@ -61,15 +61,19 @@ LogSystem::LogSystem() :
        { JOURNALD, []{ return static_cast<AbstractLogProvider *>(new JournalLogProvider()); } }
 })
 {
-       SetLogLevel(getenv(CKM_LOG_LEVEL));
+       char* level = getenv(CKM_LOG_LEVEL);
+       SetLogLevel(level ? level : "");
 
        AbstractLogProvider *prv = NULL;
 
-       try {
-               prv = m_providerCtor.at(getenv(CKM_LOG_PROVIDER))();
-       } catch (const std::exception &) {
-               prv = m_providerCtor[DLOG]();
+       char* provider = getenv(CKM_LOG_PROVIDER);
+       if (provider) {
+               auto it = m_providerCtor.find(provider);
+               if (it != m_providerCtor.end())
+                       prv = it->second();
        }
+       if (!prv)
+               prv = m_providerCtor[DLOG]();
 
        AddProvider(prv);
 }
@@ -104,7 +108,7 @@ void LogSystem::SelectProvider(const std::string &name)
        AddProvider(prv());
 }
 
-void LogSystem::SetLogLevel(const char *level)
+void LogSystem::SetLogLevel(const std::string& level)
 {
        try {
                m_level = static_cast<AbstractLogProvider::LogLevel>(std::stoi(level));

diff --git a/unit-tests/test_log-provider.cpp b/unit-tests/test_log-provider.cpp
index 5354d81..8a8c460 100644 (file)
--- a/unit-tests/test_log-provider.cpp
+++ b/unit-tests/test_log-provider.cpp
@@ -13,6 +13,8 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License
  */
+#include <cstdlib>
+
 #include <boost_macros_wrapper.h>
 
 #include <dpl/log/old_style_log_provider.h>
@@ -43,6 +45,57 @@ void testProvider(AbstractLogProvider &provider)
        BOOST_REQUIRE_NO_THROW(provider.SetTag("tag"));
 }
 
+constexpr AbstractLogProvider::LogLevel trimLogLevel(AbstractLogProvider::LogLevel level)
+{
+#ifndef BUILD_TYPE_DEBUG
+       if (level > AbstractLogProvider::LogLevel::Error)
+               level = AbstractLogProvider::LogLevel::Error;
+#endif
+       return level;
+}
+
+constexpr AbstractLogProvider::LogLevel defaultLogLevel()
+{
+#ifdef BUILD_TYPE_DEBUG
+       return AbstractLogProvider::LogLevel::Debug;
+#else
+       return  AbstractLogProvider::LogLevel::Error;
+#endif
+}
+
+
+class Env
+{
+public:
+       explicit Env(const std::string& name) : name(name)
+       {
+               char* val = getenv(name.c_str());
+               if (val)
+                       backup = val;
+       }
+
+       void Set(const std::string& value)
+       {
+               BOOST_REQUIRE(0 == setenv(name.c_str(), value.c_str(), 1));
+       }
+
+       void Unset()
+       {
+               BOOST_REQUIRE(0 == unsetenv(name.c_str()));
+       }
+
+       ~Env()
+       {
+               if (backup.empty())
+                       BOOST_CHECK(0 == unsetenv(name.c_str()));
+               else
+                       BOOST_CHECK(0 == setenv(name.c_str(), backup.c_str(), 1));
+       }
+private:
+       std::string name;
+       std::string backup;
+};
+
 } // namespace anonymous
 
 BOOST_AUTO_TEST_SUITE(LOG_PROVIDER_TEST)
@@ -83,15 +136,93 @@ POSITIVE_TEST_CASE(log_system)
        BOOST_REQUIRE_NO_THROW(system.SetTag("Test"));
 
        BOOST_REQUIRE_NO_THROW(system.SetLogLevel("5"));
+       BOOST_REQUIRE(system.GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::Pedantic));
        BOOST_REQUIRE_NO_THROW(system.SetLogLevel("4"));
+       BOOST_REQUIRE(system.GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::Debug));
        BOOST_REQUIRE_NO_THROW(system.SetLogLevel("3"));
+       BOOST_REQUIRE(system.GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::Info));
        BOOST_REQUIRE_NO_THROW(system.SetLogLevel("2"));
+       BOOST_REQUIRE(system.GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::Warning));
        BOOST_REQUIRE_NO_THROW(system.SetLogLevel("1"));
+       BOOST_REQUIRE(system.GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::Error));
        BOOST_REQUIRE_NO_THROW(system.SetLogLevel("0"));
-       BOOST_REQUIRE(system.GetLogLevel() == AbstractLogProvider::LogLevel::None);
+       BOOST_REQUIRE(system.GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::None));
 
        BOOST_REQUIRE_NO_THROW(system.SelectProvider("DLOG"));
        BOOST_REQUIRE_NO_THROW(system.SelectProvider("JOURNALD"));
 }
 
+NEGATIVE_TEST_CASE(log_system)
+{
+       LogSystem system;
+
+       BOOST_REQUIRE_NO_THROW(system.SetLogLevel(""));
+       BOOST_REQUIRE(system.GetLogLevel() == defaultLogLevel());
+       BOOST_REQUIRE_NO_THROW(system.SetLogLevel("whatever"));
+       BOOST_REQUIRE(system.GetLogLevel() == defaultLogLevel());
+
+       BOOST_REQUIRE_THROW(system.SelectProvider("WRONG_PROVIDER"), std::out_of_range);
+       BOOST_REQUIRE_THROW(system.SelectProvider(""), std::out_of_range);
+}
+
+POSITIVE_TEST_CASE(env_log_provider)
+{
+       Env env("CKM_LOG_PROVIDER");
+
+       env.Set("CONSOLE");
+       BOOST_REQUIRE_NO_THROW(LogSystem());
+       env.Set("DLOG");
+       BOOST_REQUIRE_NO_THROW(LogSystem());
+       env.Set("JOURNALD");
+       BOOST_REQUIRE_NO_THROW(LogSystem());
+}
+
+NEGATIVE_TEST_CASE(env_log_provider)
+{
+       Env env("CKM_LOG_PROVIDER");
+
+       env.Unset();
+       BOOST_REQUIRE_NO_THROW(LogSystem());
+       env.Set("");
+       BOOST_REQUIRE_NO_THROW(LogSystem());
+       env.Set("WRONG_PROVIDER");
+       BOOST_REQUIRE_NO_THROW(LogSystem());
+}
+
+POSITIVE_TEST_CASE(env_log_level)
+{
+       Env env("CKM_LOG_LEVEL");
+
+       typedef std::underlying_type<AbstractLogProvider::LogLevel>::type underlying;
+       for (underlying i = 0; i < 6; i++) {
+               std::stringstream ss;
+               ss << i;
+               env.Set(ss.str().c_str());
+
+               auto level = trimLogLevel(static_cast<AbstractLogProvider::LogLevel>(i));
+               BOOST_REQUIRE(LogSystem().GetLogLevel() == level);
+       }
+}
+
+NEGATIVE_TEST_CASE(env_log_level)
+{
+       Env env("CKM_LOG_LEVEL");
+
+       env.Unset();
+       BOOST_REQUIRE(LogSystem().GetLogLevel() == defaultLogLevel());
+
+       env.Set("");
+       BOOST_REQUIRE(LogSystem().GetLogLevel() == defaultLogLevel());
+
+       env.Set("-1");
+       BOOST_REQUIRE(LogSystem().GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::None));
+
+       env.Set("6");
+       BOOST_REQUIRE(LogSystem().GetLogLevel() == trimLogLevel(AbstractLogProvider::LogLevel::Pedantic));
+
+       env.Set("WRONG_LEVEL");
+       BOOST_REQUIRE(LogSystem().GetLogLevel() == defaultLogLevel());
+}
+
+
 BOOST_AUTO_TEST_SUITE_END()