From: Kevin Day <thekevinday@gmail.com>
Date: Sun, 14 Feb 2021 23:19:08 +0000 (-0600)
Subject: Progress: controller program, address issues with invalid reads/writes.
X-Git-Tag: 0.5.3~102
X-Git-Url: https://git.kevux.org/?a=commitdiff_plain;h=7165576e358fe1cc65f192a86ad54781ebff7204;p=fll

Progress: controller program, address issues with invalid reads/writes.

I have finally identified the cause of the confusing invalid reads/writes.
It seems that using the pointers to memory address directly associated with the thread data and then accessed within a fork() call is a problem.
By copying the data to variables local to a function and then using that memory address, the read/write problems go away.

This commit only performs an immediate fix for the current problem.
What I really need to do next is to rewrite/restructure much of the code with this behavior in consideration.
There were a lot of experimental changes I performed while trying to identify this problem.

There are still some additional locking and other thread problems to solve.
These will hopefully be fixed by the planned rewrite cleanup that I need to do.
---

diff --git a/level_3/controller/c/private-rule.c b/level_3/controller/c/private-rule.c
index ecdd625..aaed5ec 100644
--- a/level_3/controller/c/private-rule.c
+++ b/level_3/controller/c/private-rule.c
@@ -518,10 +518,6 @@ extern "C" {
     f_array_length_t j = 0;
     f_array_length_t k = 0;
 
-    controller_rule_t *rule = &thread->setting->rules.array[index];
-    controller_rule_item_t *item = 0;
-    controller_rule_action_t *action = 0;
-
     // child processes should receive all signals, without blocking.
     f_signal_how_t signals = f_signal_how_t_initialize;
     f_signal_set_empty(&signals.block);
@@ -533,84 +529,100 @@ extern "C" {
 
     controller_execute_set_t execute_set = controller_macro_execute_set_t_initialize(0, &environment, &signals, 0, fl_execute_as_t_initialize);
 
-    if (rule->affinity.used) {
-      execute_set.as.affinity = &rule->affinity;
+    // when using pointers in threads and calling the exec functions, invalid reads and writes may occure.
+    // make local copies and have the pointers reference these to avoid invalid reads and writes.
+    int local_nice;
+    uid_t local_id_user;
+    gid_t local_id_group;
+
+    f_int32s_t local_affinity;
+    f_control_group_t local_control_group;
+    f_int32s_t local_id_groups;
+    f_limit_sets_t local_limits;
+    f_execute_scheduler_t local_scheduler;
+
+    if (thread->setting->rules.array[index].affinity.used) {
+      local_affinity = thread->setting->rules.array[index].affinity;
+      execute_set.as.affinity = &local_affinity;
     }
 
-    if (rule->capability) {
-      execute_set.as.capability = rule->capability;
+    if (thread->setting->rules.array[index].capability) {
+      execute_set.as.capability = thread->setting->rules.array[index].capability;
     }
 
-    if (rule->has & controller_rule_has_control_group) {
-      execute_set.as.control_group = &rule->control_group;
+    if (thread->setting->rules.array[index].has & controller_rule_has_control_group) {
+      local_control_group = thread->setting->rules.array[index].control_group;
+      execute_set.as.control_group = &local_control_group;
 
       // make sure all required cgroup directories exist.
-      if (rule->status == F_known_not) {
-        status = fll_control_group_prepare(rule->control_group);
+      if (thread->setting->rules.array[index].status == F_known_not) {
+        status = fll_control_group_prepare(thread->setting->rules.array[index].control_group);
 
         if (F_status_is_error(status)) {
           controller_error_print_locked(thread->data->error, F_status_set_fine(status), "fll_control_group_prepare", F_true, thread);
 
-          rule->status = F_status_set_error(F_failure);
+          thread->setting->rules.array[index].status = F_status_set_error(F_failure);
           return status;
         }
       }
     }
 
-    if (rule->has & controller_rule_has_group) {
-      execute_set.as.id_group = &rule->group;
+    if (thread->setting->rules.array[index].has & controller_rule_has_group) {
+      local_id_group = thread->setting->rules.array[index].group;
+      execute_set.as.id_group = &local_id_group;
 
-      if (rule->groups.used) {
-        execute_set.as.id_groups = &rule->groups;
+      if (thread->setting->rules.array[index].groups.used) {
+        local_id_groups = thread->setting->rules.array[index].groups;
+        execute_set.as.id_groups = &local_id_groups;
       }
     }
 
-    if (rule->limits.used) {
-      execute_set.as.limits = &rule->limits;
+    if (thread->setting->rules.array[index].limits.used) {
+      local_limits = thread->setting->rules.array[index].limits;
+      execute_set.as.limits = &local_limits;
     }
 
-    if (rule->has & controller_rule_has_scheduler) {
-      execute_set.as.scheduler = &rule->scheduler;
+    if (thread->setting->rules.array[index].has & controller_rule_has_scheduler) {
+      local_scheduler = thread->setting->rules.array[index].scheduler;
+      execute_set.as.scheduler = &local_scheduler;
     }
 
-    if (rule->has & controller_rule_has_nice) {
-      execute_set.as.nice = &rule->nice;
+    if (thread->setting->rules.array[index].has & controller_rule_has_nice) {
+      local_nice = thread->setting->rules.array[index].nice;
+      execute_set.as.nice = &local_nice;
     }
 
-    if (rule->has & controller_rule_has_user) {
-      execute_set.as.id_user = &rule->user;
+    if (thread->setting->rules.array[index].has & controller_rule_has_user) {
+      local_id_user = thread->setting->rules.array[index].user;
+      execute_set.as.id_user = &local_id_user;
     }
 
-    status = fl_environment_load_names(rule->environment, &environment);
+    status = fl_environment_load_names(thread->setting->rules.array[index].environment, &environment);
 
     if (F_status_is_error(status)) {
       controller_error_print_locked(thread->data->error, F_status_set_fine(status), "fl_environment_load_names", F_true, thread);
 
-      rule->status = F_status_set_error(F_failure);
+      thread->setting->rules.array[index].status = F_status_set_error(F_failure);
       return status;
     }
 
-    for (i = 0; i < rule->items.used; ++i) {
+    for (i = 0; i < thread->setting->rules.array[index].items.used; ++i) {
 
       if (thread->setting->signal) {
         status = F_signal;
         break;
       }
 
-      if (rule->items.array[i].type == controller_rule_item_type_setting) continue;
-
-      item = &rule->items.array[i];
+      if (thread->setting->rules.array[index].items.array[i].type == controller_rule_item_type_setting) continue;
 
-      for (j = 0; j < item->actions.used; ++j) {
+      for (j = 0; j < thread->setting->rules.array[index].items.array[i].actions.used; ++j) {
 
         if (thread->setting->signal) {
           status = F_signal;
           break;
         }
 
-        if (item->actions.array[j].type != type) continue;
-
-        action = &item->actions.array[j];
+        if (thread->setting->rules.array[index].items.array[i].actions.array[j].type != type) continue;
 
         execute_set.parameter.data = 0;
         execute_set.parameter.option = fl_execute_parameter_option_threadsafe;
@@ -619,55 +631,55 @@ extern "C" {
           execute_set.parameter.option |= fl_execute_parameter_option_return;
         }
 
-        if (item->type == controller_rule_item_type_command) {
+        if (thread->setting->rules.array[index].items.array[i].type == controller_rule_item_type_command) {
 
-          if (strchr(action->parameters.array[0].string, f_path_separator_s[0])) {
+          if (strchr(thread->setting->rules.array[index].items.array[i].actions.array[j].parameters.array[0].string, f_path_separator_s[0])) {
             execute_set.parameter.option |= fl_execute_parameter_option_path;
           }
 
-          status = controller_rule_execute_foreground(index, item->type, *action, 0, action->parameters, options, &execute_set, thread, asynchronous);
+          status = controller_rule_execute_foreground(index, thread->setting->rules.array[index].items.array[i].type, thread->setting->rules.array[index].items.array[i].actions.array[j], 0, thread->setting->rules.array[index].items.array[i].actions.array[j].parameters, options, &execute_set, thread, asynchronous);
 
           if (status == F_child) break;
 
           if (F_status_is_error(status)) {
-            action->status = F_status_set_error(F_failure);
+            thread->setting->rules.array[index].items.array[i].actions.array[j].status = F_status_set_error(F_failure);
 
             if (!(options & controller_rule_option_simulate)) break;
           }
 
           success = F_true;
         }
-        else if (item->type == controller_rule_item_type_script) {
-          execute_set.parameter.data = &action->parameters.array[0];
+        else if (thread->setting->rules.array[index].items.array[i].type == controller_rule_item_type_script) {
+          execute_set.parameter.data = &thread->setting->rules.array[index].items.array[i].actions.array[j].parameters.array[0];
 
-          if (rule->script.used && strchr(rule->script.string, f_path_separator_s[0])) {
+          if (thread->setting->rules.array[index].script.used && strchr(thread->setting->rules.array[index].script.string, f_path_separator_s[0])) {
             execute_set.parameter.option |= fl_execute_parameter_option_path;
           }
 
-          status = controller_rule_execute_foreground(index, item->type, *action, rule->script.used ? rule->script.string : controller_default_program_script, arguments_none, options, &execute_set, thread, asynchronous);
+          status = controller_rule_execute_foreground(index, thread->setting->rules.array[index].items.array[i].type, thread->setting->rules.array[index].items.array[i].actions.array[j], thread->setting->rules.array[index].script.used ? thread->setting->rules.array[index].script.string : controller_default_program_script, arguments_none, options, &execute_set, thread, asynchronous);
 
           if (status == F_child) break;
 
           if (F_status_is_error(status)) {
-            action->status = F_status_set_error(F_failure);
+            thread->setting->rules.array[index].items.array[i].actions.array[j].status = F_status_set_error(F_failure);
 
             if (!(options & controller_rule_option_simulate)) break;
           }
 
           success = F_true;
         }
-        else if (item->type == controller_rule_item_type_service) {
+        else if (thread->setting->rules.array[index].items.array[i].type == controller_rule_item_type_service) {
 
-          if (strchr(action->parameters.array[0].string, f_path_separator_s[0])) {
+          if (strchr(thread->setting->rules.array[index].items.array[i].actions.array[j].parameters.array[0].string, f_path_separator_s[0])) {
             execute_set.parameter.option |= fl_execute_parameter_option_path;
           }
 
-          status = controller_rule_execute_pid_with(index, item->type, *action, 0, action->parameters, options, &execute_set, thread, asynchronous);
+          status = controller_rule_execute_pid_with(index, thread->setting->rules.array[index].items.array[i].type, thread->setting->rules.array[index].items.array[i].actions.array[j], 0, thread->setting->rules.array[index].items.array[i].actions.array[j].parameters, options, &execute_set, thread, asynchronous);
 
           if (status == F_child) break;
 
           if (F_status_is_error(status)) {
-            action->status = F_status_set_error(F_failure);
+            thread->setting->rules.array[index].items.array[i].actions.array[j].status = F_status_set_error(F_failure);
 
             if (!(options & controller_rule_option_simulate)) break;
           }
@@ -687,7 +699,7 @@ extern "C" {
             f_thread_mutex_unlock(&thread->mutex->print);
           }
 
-          action->status = F_ignore;
+          thread->setting->rules.array[index].items.array[i].actions.array[j].status = F_ignore;
 
           if (success != F_true) {
             success = F_ignore;
@@ -709,16 +721,16 @@ extern "C" {
     }
 
     if (F_status_is_error(status) || success == F_false) {
-      rule->status = F_status_set_error(F_failure);
+      thread->setting->rules.array[index].status = F_status_set_error(F_failure);
     }
     else if (success == F_ignore || success == F_busy) {
-      rule->status = success;
+      thread->setting->rules.array[index].status = success;
     }
     else {
-      rule->status = F_none;
+      thread->setting->rules.array[index].status = F_none;
     }
 
-    return rule->status;
+    return thread->setting->rules.array[index].status;
   }
 #endif // _di_controller_rule_execute_