Fix double free in label monitor

The pointer passed to initialize_inotify() is already managed by a unique_ptr.
In case of initialize_inotify() failure the pointer was being passed to
security_manager_app_labels_monitor_finish() where it was wrapped in a
unique_ptr again.

Add a helper non-throwing function operating on raw app_labels_monitor pointer
for properly closing it. Use it in security_manager_app_labels_monitor_finish()
and security_manager_app_labels_monitor_init().

Change-Id: I6f3b5883fde53ba3ded7764f3121f59ace053a10

diff --git a/src/client/client-label-monitor.cpp b/src/client/client-label-monitor.cpp
index a92f1513ae0717d699f513490aee47ac0ee7c5ef..3b3ec5ba69eaab02071667516e9944c91735af14 100644 (file)
--- a/src/client/client-label-monitor.cpp
+++ b/src/client/client-label-monitor.cpp
@@ -104,20 +104,44 @@ static lib_retcode initialize_inotify(app_labels_monitor *monitor)
     monitor->inotify = ret;
     ret_lib = inotify_add_watch_full(monitor->inotify, monitor->global_label_file_path.c_str(),
             IN_CLOSE_WRITE | IN_DELETE_SELF, &(monitor->global_labels_file_watch));
-    if (ret_lib != SECURITY_MANAGER_SUCCESS) {
-        security_manager_app_labels_monitor_finish(monitor);
+    if (ret_lib != SECURITY_MANAGER_SUCCESS)
         return ret_lib;
-    }
+
     ret_lib = inotify_add_watch_full(monitor->inotify, monitor->user_label_file_path.c_str(),
             IN_CLOSE_WRITE | IN_DELETE_SELF, &(monitor->user_labels_file_watch));
-    if (ret_lib != SECURITY_MANAGER_SUCCESS) {
-        security_manager_app_labels_monitor_finish(monitor);
+    if (ret_lib != SECURITY_MANAGER_SUCCESS)
         return ret_lib;
-    }
 
     return SECURITY_MANAGER_SUCCESS;
 }
 
+static void close_inotify(app_labels_monitor *monitor) noexcept
+{
+    if (monitor->inotify == -1)
+        return;
+
+    if (monitor->global_labels_file_watch != -1) {
+        int ret = inotify_rm_watch(monitor->inotify, monitor->global_labels_file_watch);
+        if (ret == -1) {
+            try {
+                LogErrno("Inotify watch removal on file " << APPS_LABELS_FILE);
+            } catch (...) {}
+        }
+        monitor->global_labels_file_watch = -1;
+    }
+    if (monitor->user_labels_file_watch != -1) {
+        int ret = inotify_rm_watch(monitor->inotify, monitor->user_labels_file_watch);
+        if (ret == -1) {
+            try {
+                LogErrno("Inotify watch removal on file " << monitor->user_label_file_path);
+            } catch (...) {}
+        }
+        monitor->user_labels_file_watch = -1;
+    }
+    close(monitor->inotify);
+    delete monitor;
+}
+
 static void readPermissibleFile(std::vector<std::string> &appLabels, const std::string &filePath, app_install_type installType, uid_t uid)
 {
     try {
@@ -177,7 +201,7 @@ int security_manager_app_labels_monitor_init(app_labels_monitor **monitor)
 
         *monitor = nullptr;
 
-        auto monitorPtr = makeUnique(new app_labels_monitor, security_manager_app_labels_monitor_finish);
+        auto monitorPtr = makeUnique(new app_labels_monitor, close_inotify);
         if (!monitorPtr) {
             LogError("Bad memory allocation for app_labels_monitor");
             return SECURITY_MANAGER_ERROR_MEMORY;
@@ -206,33 +230,14 @@ int security_manager_app_labels_monitor_init(app_labels_monitor **monitor)
 SECURITY_MANAGER_API
 void security_manager_app_labels_monitor_finish(app_labels_monitor *monitor)
 {
-    try_catch([&] {
+    (void)try_catch([&] {
         LogDebug("security_manager_app_labels_monitor_finish() called");
         if (monitor == nullptr) {
             LogDebug("input param \"monitor\" is nullptr");
-            return 0;
         }
-        auto monitorPtr = makeUnique(monitor);
-        if (monitorPtr->inotify != -1) {
-            if (monitorPtr->global_labels_file_watch != -1) {
-                int ret = inotify_rm_watch(monitorPtr->inotify, monitorPtr->global_labels_file_watch);
-                if (ret == -1) {
-                    LogErrno("Inotify watch removal on file " << APPS_LABELS_FILE);
-                }
-                monitorPtr->global_labels_file_watch = -1;
-            }
-            if (monitorPtr->user_labels_file_watch != -1) {
-                int ret = inotify_rm_watch(monitorPtr->inotify, monitorPtr->user_labels_file_watch);
-                if (ret == -1) {
-                    LogErrno("Inotify watch removal on file " << monitor->user_label_file_path);
-                }
-                monitorPtr->user_labels_file_watch = -1;
-            }
-            close(monitorPtr->inotify);
-            monitorPtr->inotify = -1;
-        }
-        return 0;
+        return SECURITY_MANAGER_SUCCESS;
     });
+    close_inotify(monitor);
 }
 
 SECURITY_MANAGER_API