security-manager: Fix segfaults in PolicyRequest

Change-Id: I1273c17991d4c32d5585740b2386d5e04b4e086f

diff --git a/src/common/sm_api.cpp b/src/common/sm_api.cpp
index 478e0c6adfaf4bcc162fb93f736a185be0eb59fa..d9ed32c29193b77e53eb10cd4fadf4251a3dba76 100644 (file)
--- a/src/common/sm_api.cpp
+++ b/src/common/sm_api.cpp
@@ -170,10 +170,21 @@ void getConfiguredPolicy(const PolicyEntry &filter, std::vector<PolicyEntry> &po
     size_t policy_size = 0;
     int result;
 
+    policy_entry *entry_p;
+    result = security_manager_policy_entry_new(&entry_p);
+    RUNNER_ASSERT_MSG((lib_retcode)result == SECURITY_MANAGER_SUCCESS,
+                      "creation of new policy entry failed. Result: " << result);
+    RUNNER_ASSERT_MSG(entry_p != nullptr, "creation of new policy entry did not allocate memory");
+
+    auto entryPtr = std::unique_ptr<policy_entry, decltype(&security_manager_policy_entry_free)>
+        {entry_p, security_manager_policy_entry_free};
+
+    filter.fill(entry_p);
+
     if (forAdmin) {
-        result = security_manager_get_configured_policy_for_admin(filter.get(), &pp_privs_policy, &policy_size);
+        result = security_manager_get_configured_policy_for_admin(entry_p, &pp_privs_policy, &policy_size);
     } else {
-        result = security_manager_get_configured_policy_for_self(filter.get(), &pp_privs_policy, &policy_size);
+        result = security_manager_get_configured_policy_for_self(entry_p, &pp_privs_policy, &policy_size);
     };
 
     RUNNER_ASSERT_MSG((lib_retcode)result == expectedResult,
@@ -181,7 +192,8 @@ void getConfiguredPolicy(const PolicyEntry &filter, std::vector<PolicyEntry> &po
                           << " Result: " << result << ";");
 
     for (unsigned int i = 0; i < policy_size; ++i) {
-        PolicyEntry pe(*pp_privs_policy[i]);
+
+        PolicyEntry pe(pp_privs_policy[i]);
         policyEntries.push_back(pe);
     };
 }
@@ -192,12 +204,23 @@ void getPolicy(const PolicyEntry &filter, std::vector<PolicyEntry> &policyEntrie
     size_t policy_size = 0;
     int result;
 
-    result = security_manager_get_policy(filter.get(), &pp_privs_policy, &policy_size);
+    policy_entry *entry_p;
+    result = security_manager_policy_entry_new(&entry_p);
+    RUNNER_ASSERT_MSG((lib_retcode)result == SECURITY_MANAGER_SUCCESS,
+                      "creation of new policy entry failed. Result: " << result);
+    RUNNER_ASSERT_MSG(entry_p != nullptr, "creation of new policy entry did not allocate memory");
+
+    auto entryPtr = std::unique_ptr<policy_entry, decltype(&security_manager_policy_entry_free)>
+        {entry_p, security_manager_policy_entry_free};
+
+    filter.fill(entry_p);
+
+    result = security_manager_get_policy(entry_p, &pp_privs_policy, &policy_size);
     RUNNER_ASSERT_MSG((lib_retcode)result == expectedResult,
                       "Unexpected result" << std::endl
                           << " Result: " << result << ";");
     for (unsigned int i = 0; i < policy_size; ++i) {
-        PolicyEntry pe(*pp_privs_policy[i]);
+        PolicyEntry pe(pp_privs_policy[i]);
         policyEntries.push_back(pe);
     };
 }

diff --git a/src/common/sm_policy_request.cpp b/src/common/sm_policy_request.cpp
index c129a76aa885b8f134e009267034251470242b6a..fac911c23e57341b428498ff6f1b00b8b46c0b95 100644 (file)
--- a/src/common/sm_policy_request.cpp
+++ b/src/common/sm_policy_request.cpp
@@ -14,6 +14,8 @@
  *    limitations under the License.
  */
 
+#include <memory>
+#include <sstream>
 #include <sm_policy_request.h>
 
 #include <dpl/test/test_runner.h>
@@ -25,128 +27,102 @@ constexpr char PolicyEntry::LEVEL_DENY[];
 constexpr char PolicyEntry::LEVEL_ASK_USER[];
 
 PolicyEntry::PolicyEntry()
-    : m_appId(true, std::string(SECURITY_MANAGER_ANY))
-    , m_user(true, std::string(SECURITY_MANAGER_ANY))
-    , m_privilege(true, std::string(SECURITY_MANAGER_ANY))
-    , m_currentLevel(false, std::string(""))
-    , m_maxLevel(false, std::string(""))
-{
-    int result = security_manager_policy_entry_new(&m_entry);
-    RUNNER_ASSERT_MSG((lib_retcode)result == SECURITY_MANAGER_SUCCESS,
-                      "creation of new policy entry failed. Result: " << result);
-    RUNNER_ASSERT_MSG(m_entry != nullptr, "creation of new policy entry did not allocate memory");
-
-    security_manager_policy_entry_set_application(m_entry, m_appId.second.c_str());
-    security_manager_policy_entry_set_user(m_entry, m_user.second.c_str());
-    security_manager_policy_entry_set_privilege(m_entry, m_privilege.second.c_str());
-}
+    : m_appId(std::string(SECURITY_MANAGER_ANY))
+    , m_user(std::string(SECURITY_MANAGER_ANY))
+    , m_privilege(std::string(SECURITY_MANAGER_ANY))
+{}
 
 PolicyEntry::PolicyEntry(const std::string &appId, const std::string &user,
         const std::string &privilege)
-    : m_appId(true, std::string(appId))
-    , m_user(true, std::string(user))
-    , m_privilege(true, std::string(privilege))
-    , m_currentLevel(false, std::string(""))
-    , m_maxLevel(false, std::string(""))
+    : m_appId(appId)
+    , m_user(user)
+    , m_privilege(privilege)
+{}
+
+PolicyEntry::PolicyEntry(policy_entry *entry)
 {
-    int result = security_manager_policy_entry_new(&m_entry);
-    RUNNER_ASSERT_MSG((lib_retcode)result == SECURITY_MANAGER_SUCCESS,
-                      "creation of new policy entry failed. Result: " << result);
-    RUNNER_ASSERT_MSG(m_entry != nullptr, "creation of new policy entry did not allocate memory");
+    constexpr static auto cstr_to_opt = [](const char* cstr) {
+        return cstr ? std::optional<std::string>{cstr} : std::nullopt;
+    };
 
-    security_manager_policy_entry_set_user(m_entry, m_user.second.c_str());
-    security_manager_policy_entry_set_application(m_entry, m_appId.second.c_str());
-    security_manager_policy_entry_set_privilege(m_entry, m_privilege.second.c_str());
-}
 
-PolicyEntry::PolicyEntry(policy_entry &entry): m_entry(&entry)
-{
-    m_appId.first = true;
-    m_appId.second = std::string(security_manager_policy_entry_get_application(m_entry));
+    m_appId = cstr_to_opt(security_manager_policy_entry_get_application(entry));
 
-    m_user.first = true;
-    m_user.second = std::string(security_manager_policy_entry_get_user(m_entry));
+    m_user = cstr_to_opt(security_manager_policy_entry_get_user(entry));
 
-    m_privilege.first = true;
-    m_privilege.second = std::string(security_manager_policy_entry_get_privilege(m_entry));
+    m_privilege = cstr_to_opt(security_manager_policy_entry_get_privilege(entry));
 
-    m_currentLevel.first = true;
-    m_currentLevel.second = std::string(security_manager_policy_entry_get_level(m_entry));
+    m_currentLevel = cstr_to_opt(security_manager_policy_entry_get_level(entry));
 
-    m_maxLevel.first = true;
-    m_maxLevel.second = std::string(security_manager_policy_entry_get_max_level(m_entry));
+    m_maxLevel = cstr_to_opt(security_manager_policy_entry_get_max_level(entry));
 };
 
 void PolicyEntry::setLevel(const std::string &level)
 {
-    m_currentLevel.first = true;
-    m_currentLevel.second = level;
-    security_manager_policy_entry_set_level(m_entry, level.c_str());
-    m_maxLevel.first = true;
-    m_maxLevel.second = std::string(security_manager_policy_entry_get_max_level(m_entry));
+    m_currentLevel = level;
 };
 
 void PolicyEntry::setMaxLevel(const std::string &level)
 {
-    m_maxLevel.first = true;
-    m_maxLevel.second = level;
-    security_manager_policy_entry_admin_set_level(m_entry, level.c_str());
-    m_currentLevel.first = true;
-    m_currentLevel.second = std::string(security_manager_policy_entry_get_level(m_entry));
+    m_maxLevel = level;
 };
 
+void PolicyEntry::fill(policy_entry *entry_p) const
+{
+    if (m_appId) {
+        security_manager_policy_entry_set_application(entry_p, m_appId->c_str());
+    }
+    if (m_user) {
+        security_manager_policy_entry_set_user(entry_p, m_user->c_str());
+    }
+    if (m_privilege) {
+        security_manager_policy_entry_set_privilege(entry_p, m_privilege->c_str());
+    }
+    if (m_currentLevel) {
+        security_manager_policy_entry_set_level(entry_p, m_currentLevel->c_str());
+    }
+    if (m_maxLevel) {
+        security_manager_policy_entry_admin_set_level(entry_p, m_maxLevel->c_str());
+    }
+}
 
 std::ostream& operator<<(std::ostream &os, const PolicyEntry &request)
 {
-    if (request.m_appId.first)
-            os << "appId: " << request.m_appId.second << "; ";
+    if (request.m_appId)
+            os << "appId: " << *request.m_appId << "; ";
 
-    if (request.m_user.first)
-            os << "user: " << request.m_user.second << "; ";
+    if (request.m_user)
+            os << "user: " << *request.m_user << "; ";
 
-    if (request.m_privilege.first)
-            os << "privilege: " << request.m_privilege.second << "; ";
+    if (request.m_privilege)
+            os << "privilege: " << *request.m_privilege << "; ";
 
-    if (request.m_currentLevel.first)
-            os << "current: " << request.m_currentLevel.second << "; ";
+    if (request.m_currentLevel)
+            os << "current: " << *request.m_currentLevel << "; ";
 
-    if (request.m_maxLevel.first)
-            os << "max: " << request.m_maxLevel.second << "; ";
+    if (request.m_maxLevel)
+            os << "max: " << *request.m_maxLevel << "; ";
 
     return os;
 }
 
-PolicyEntry::~PolicyEntry()
-{
-}
-
-void PolicyEntry::free(void)
-{
-    security_manager_policy_entry_free(m_entry);
-}
-
 bool PolicyEntry::operator==(const PolicyEntry &other) const
 {
-    auto cmp = [](const std::pair<bool, std::string> &a, const std::pair<bool, std::string> &b)->bool
-    {
-        return (a.first) ? (b.first && a.second == b.second) : !b.first;
-    };
-
-    return (
-        cmp(m_appId, other.m_appId) &&
-        cmp(m_user, other.m_user) &&
-        cmp(m_privilege, other.m_privilege) &&
-        cmp(m_currentLevel, other.m_currentLevel) &&
-        cmp(m_maxLevel, other.m_maxLevel));
+    return
+        m_appId == other.m_appId &&
+        m_user == other.m_user &&
+        m_privilege == other.m_privilege &&
+        m_currentLevel == other.m_currentLevel &&
+        m_maxLevel == other.m_maxLevel;
 }
 
-std::string PolicyEntry::toString() const
+std::string PolicyEntry::toHashKey() const
 {
     std::stringstream ss;
-    auto append = [&](const std::pair<bool, std::string> &x)
+    auto append = [&](const std::optional<std::string> &x)
     {
-        if (x.first)
-            ss << x.second;
+        if (x)
+            ss << *x;
         ss << '\0';
     };
 
@@ -171,18 +147,27 @@ PolicyRequest::PolicyRequest()
 
 PolicyRequest::~PolicyRequest()
 {
-    for(std::vector<PolicyEntry>::iterator it = m_entries.begin(); it != m_entries.end(); ++it) {
-        it->free();
+    for (auto entry_p: m_entries) {
+        security_manager_policy_entry_free(entry_p);
     }
     security_manager_policy_update_req_free(m_req);
 }
 
 void PolicyRequest::addEntry(PolicyEntry &entry,
-        lib_retcode expectedResult)
+                             lib_retcode expectedResult)
 {
-    int result = 0;
+    policy_entry *entry_p;
+    int result = security_manager_policy_entry_new(&entry_p);
+    RUNNER_ASSERT_MSG((lib_retcode)result == SECURITY_MANAGER_SUCCESS,
+                      "creation of new policy entry failed. Result: " << result);
+    RUNNER_ASSERT_MSG(entry_p != nullptr, "creation of new policy entry did not allocate memory");
+
+    auto entryPtr = std::unique_ptr<policy_entry, decltype(&security_manager_policy_entry_free)>
+        {entry_p, security_manager_policy_entry_free};
+
+    entry.fill(entry_p);
 
-    result = security_manager_policy_update_req_add_entry(m_req, entry.get());
+    result = security_manager_policy_update_req_add_entry(m_req, entry_p);
 
     RUNNER_ASSERT_MSG((lib_retcode)result == expectedResult,
                      "adding policy entry to request returned wrong value."
@@ -190,7 +175,8 @@ void PolicyRequest::addEntry(PolicyEntry &entry,
                           << " Result: " << result << ";"
                           << " Expected result: " << expectedResult);
 
-    m_entries.push_back(entry);
+    m_entries.push_back(entry_p);
+    entryPtr.release();
 }
 
 std::ostream& operator<<(std::ostream &os, const PolicyRequest &request)
@@ -200,7 +186,7 @@ std::ostream& operator<<(std::ostream &os, const PolicyRequest &request)
         os << "PolicyRequest m_entries size: " << request.m_entries.size() << "; ";
 
         for(unsigned int i = 0; i != request.m_entries.size(); i++) {
-           os << "entry " << i << ": " << request.m_entries[i] << "; ";
+            os << "entry " << i << ": " << PolicyEntry(request.m_entries[i]) << "; ";
        }
     }
 

diff --git a/src/common/sm_policy_request.h b/src/common/sm_policy_request.h
index d1791a7d88f699cec6b16d3060d96b98d8138754..77d232364e183b862e33fc5ea44bea24b808324b 100644 (file)
--- a/src/common/sm_policy_request.h
+++ b/src/common/sm_policy_request.h
@@ -18,6 +18,7 @@
 #define SECURITY_MANAGER_TEST_POLICYREQUEST
 
 #include <iostream>
+#include <optional>
 #include <sys/types.h>
 #include <utility>
 #include <vector>
@@ -39,31 +40,34 @@ public:
                 const std::string &user,
                 const std::string &privilege
                 );
-    ~PolicyEntry();
-
-    PolicyEntry(policy_entry &entry);
-
-    policy_entry *get() const { return m_entry; }
-    std::string getUser() const         { return m_user.second; }
-    std::string getAppId() const        { return m_appId.second; }
-    std::string getPrivilege() const    { return m_privilege.second; }
-    std::string getCurrentLevel() const { return m_currentLevel.second; }
-    std::string getMaxLevel() const     { return m_maxLevel.second; }
+    ~PolicyEntry() = default;
+
+    PolicyEntry(policy_entry *entry);
+
+    bool hasUser() const                { return m_user.has_value(); }
+    bool hasAppId() const               { return m_appId.has_value(); }
+    bool hasPrivilege() const           { return m_privilege.has_value(); }
+    bool hasCurrentLevel() const        { return m_currentLevel.has_value(); }
+    bool hasMaxLevel() const            { return m_maxLevel.has_value(); }
+    std::string getUser() const         { return m_user.value_or(""); }
+    std::string getAppId() const        { return m_appId.value_or(""); }
+    std::string getPrivilege() const    { return m_privilege.value_or(""); }
+    std::string getCurrentLevel() const { return m_currentLevel.value_or(""); }
+    std::string getMaxLevel() const     { return m_maxLevel.value_or(""); }
     void setLevel(const std::string &level);
     void setMaxLevel(const std::string &level);
-    void free(void);
+    void fill(policy_entry *entry) const;
 
     friend std::ostream& operator<<(std::ostream &, const PolicyEntry&);
     bool operator==(const PolicyEntry &) const;
-    std::string toString() const;
+    std::string toHashKey() const;
 
 private:
-    policy_entry *m_entry;
-    std::pair<bool, std::string> m_appId;
-    std::pair<bool, std::string> m_user;
-    std::pair<bool, std::string> m_privilege;
-    std::pair<bool, std::string> m_currentLevel;
-    std::pair<bool, std::string> m_maxLevel;
+    std::optional<std::string> m_appId;
+    std::optional<std::string> m_user;
+    std::optional<std::string> m_privilege;
+    std::optional<std::string> m_currentLevel;
+    std::optional<std::string> m_maxLevel;
 };
 
 std::ostream& operator<<(std::ostream &os, const SecurityManagerTest::PolicyEntry &request);
@@ -83,7 +87,7 @@ public:
 
 private:
     policy_update_req *m_req;
-    std::vector<PolicyEntry> m_entries;
+    std::vector<policy_entry *> m_entries;
 };
 
 std::ostream& operator<<(std::ostream &os, const SecurityManagerTest::PolicyRequest &request);
@@ -94,7 +98,9 @@ namespace std {
 
 template<>
 struct hash<SecurityManagerTest::PolicyEntry> {
-    size_t operator()(const SecurityManagerTest::PolicyEntry &x) const { return hash<string>()(x.toString()); }
+    size_t operator()(const SecurityManagerTest::PolicyEntry &x) const {
+        return hash<string>()(x.toHashKey());
+    }
 };
 
 } // namespace std