Bugs 519273 & 346342 - Clean Utils::File::run() logic by using array of arguments

Using system() with a string instead of an array raises various issues when strings passed include special chars. Overall, it's claner and safer to use an array everywhere. This requires us to use our own execution code since system() does not accept setting STDOUT, STDERR, LC_ALL, and starting processes in the background when using an array. Note that run_backticks() still use the old logic, which is safe(r) because those commandlines are static. However, this commit also cleans get_cmd_path() by merging do_get_cmd_path() into it, and removing duplicate settings like $PATH.

2 files changed, 95 insertions, 96 deletions

diff --git a/Users/Groups.pm b/Users/Groups.pm
index c19053f..facb7a6 100644
--- a/Users/Groups.pm
+++ b/Users/Groups.pm
@@ -52,15 +52,15 @@ sub del_group
 
   if ($Utils::Backend::tool{"system"} eq "FreeBSD")
   {
-    $command = "$cmd_pw groupdel -n \'" . $$group[$LOGIN] . "\'";
+    @command = ($cmd_pw, "groupdel", "-n", $$group[$LOGIN]);
   }
   else
   {
-    $command  = ($cmd_delgroup) ? $cmd_delgroup : $cmd_groupdel;
-    $command .= " \'" . $$group[$LOGIN] . "\'";
+    @command  = (($cmd_delgroup) ? $cmd_delgroup : $cmd_groupdel,
+                 $$group[$LOGIN]);
   }
 
-  &Utils::File::run ($command);
+  &Utils::File::run (@command);
 }
 
 # This is only for Linux and SunOS,
@@ -68,7 +68,7 @@ sub del_group
 sub add_user_to_group
 {
   my ($group, $user) = @_;
-  my ($command);
+  my (@command);
 
   if ($Utils::Backend::tool{"system"} eq "SunOS")
   {
@@ -84,14 +84,14 @@ sub add_user_to_group
     $groups =~ s/^,//;
     $groups =~ s/,$//;
 
-    $command = "$cmd_usermod -G $groups $user";
+    @command = ($cmd_usermod, "-G", $groups, $user);
   }
   else
   {
-    $command = "$cmd_gpasswd -a \'" . $user . "\' " . $group;
+    @command = ($cmd_gpasswd, "-a", $user, $group);
   }
 
-  &Utils::File::run ($command);
+  &Utils::File::run (@command);
 }
 
 # This is only for Linux and SunOS,
@@ -99,11 +99,11 @@ sub add_user_to_group
 sub delete_user_from_group
 {
   my ($group, $user) = @_;
-  my ($command);
+  my (@command);
 
   if ($Utils::Backend::tool{"system"} eq "SunOS")
   {
-    my ($groups, @arr);
+    my ($groups, @groups_arr);
 
     $groups = &Utils::File::run_backtick ("groups $user");
     $groups =~ s/.*://;
@@ -112,52 +112,47 @@ sub delete_user_from_group
     # delete the user
     $groups =~ s/[ \t]+$group//;
 
-    @arr = split (/ /, $groups);
-    $groups = join (',', @arr);
-    $groups =~ s/^,//;
-    $groups =~ s/,$//;
+    @groups_arr = split (/ /, $groups);
     
-    $command = "$cmd_usermod -G $groups $user";
+    @command = ($cmd_usermod, "-G", @groups_arr, $user);
   }
   else
   {
-    $command = "$cmd_gpasswd -d \'" . $user . "\' \'" . $group . "\'";
+    @command = ($cmd_gpasswd, "-d", $user, $group);
   }
 
-  &Utils::File::run ($command);
+  &Utils::File::run (@command);
 }
 
 sub add_group
 {
   my ($group) = @_;
-  my ($u, $user, $users);
+  my ($u, $user, @users);
 
   $u = $$group[$USERS];
 
   if ($Utils::Backend::tool{"system"} eq "FreeBSD")
   {
-    $users = join (",", sort @$u);
+    @users = sort @$u;
       
-    $command = "$cmd_pw groupadd -n \'" . $$group[$LOGIN] .
-      "\' -g \'" . $$group[$GID] .
-      "\' -M \'" . $users . "\'";
+    @command = ($cmd_pw, "groupadd", "-n", $$group[$LOGIN],
+                                     "-g", $$group[$GID],
+                                     "-M", @users);
 
-    &Utils::File::run ($command);
+    &Utils::File::run (@command);
   }
   else
   {
     if ($cmd_addgroup)
     {
-      $command = "$cmd_addgroup " .
-          "--gid \'" . $$group[$GID] . "\' " . $$group[$LOGIN];
+      @command = ($cmd_addgroup, "--gid", $$group[$GID], $$group[$LOGIN]);
     }
     else
     {
-      $command = "$cmd_groupadd -g \'" . $$group[$GID] .
-          "\' " . $$group[$LOGIN];
+      @command = ($cmd_groupadd, "-g", $$group[$GID], $$group[$LOGIN]);
     }
 
-    &Utils::File::run ($command);
+    &Utils::File::run (@command);
 
     foreach $user (sort @$u)
     {
@@ -178,20 +173,20 @@ sub change_group
     $users_arr = $$new_group[$USERS];
     $str = join (",", sort @$users_arr);
 
-    $command = "$cmd_pw groupmod -n \'" . $$old_group[$LOGIN] .
-        "\' -g \'" . $$new_group[$GID] .
-        "\' -l \'" . $$new_group[$LOGIN] .
-        "\' -M \'" . $str . "\'";
+    @command = ($cmd_pw, "groupmod", "-n", $$old_group[$LOGIN],
+                                     "-g", $$new_group[$GID],
+                                     "-l", $$new_group[$LOGIN],
+                                     "-M", $str);
 
-    &Utils::File::run ($command);
+    &Utils::File::run (@command);
   }
   else
   {
-    $command = "$cmd_groupmod -g \'" . $$new_group[$GID] .
-        "\' -n \'" . $$new_group[$LOGIN] . "\' " .
-        "\'" . $$old_group[$LOGIN] . "\'";
+    @command = ($cmd_groupmod, "-g", $$new_group[$GID],
+                               "-n", $$new_group[$LOGIN],
+                                     $$old_group[$LOGIN]);
   
-    &Utils::File::run ($command);
+    &Utils::File::run (@command);
 
     # Let's see if the users that compose the group have changed.
     if (!Utils::Util::struct_eq ($$new_group[$USERS], $$old_group[$USERS]))
diff --git a/Users/Users.pm b/Users/Users.pm
index bd0603a..bfc6d96 100644
--- a/Users/Users.pm
+++ b/Users/Users.pm
@@ -439,32 +439,32 @@ sub get
 sub del_user
 {
 	my ($user) = @_;
-  my ($command);
+  my (@command);
 	
   if ($Utils::Backend::tool{"system"} eq "FreeBSD")
   {
-    $command = "$cmd_pw userdel -n \'" . $$user[$LOGIN] . "\' ";
+    @command = ($cmd_pw, "userdel", "-n", $$user[$LOGIN]);
   }
   else
   {
     if ($cmd_deluser)
     {
-      $command = "$cmd_deluser '". $$user[$LOGIN] . "'";
+      @command = ($cmd_deluser, $$user[$LOGIN]);
     }
     else
     {
-      $command = "$cmd_userdel \'" . $$user[$LOGIN] . "\'";
+      @command = ($cmd_userdel, $$user[$LOGIN]);
     }
   }
 
-  &Utils::File::run ($command);
+  &Utils::File::run (@command);
 }
 
 sub change_user_chfn
 {
   my ($login, $old_comment, $comment) = @_;
   my ($fname, $office, $office_phone, $home_phone);
-  my ($command, $str);
+  my (@command, $str);
 
   return if !$login;
 
@@ -474,14 +474,15 @@ sub change_user_chfn
 
   if ($Utils::Backend::tool{"system"} eq "FreeBSD")
   {
-    $command = "$cmd_pw usermod -n " . $login . " -c \'" . $str . "\'";
+    @command = ($cmd_pw, "usermod", "-n", $login,
+                                    "-c", $str);
   }
   else
   {
-    $command = "$cmd_usermod -c \'" . $str . "\' " . $login;
+    @command = ($cmd_usermod, "-c", $str, $login);
   }
 
-  &Utils::File::run ($command);
+  &Utils::File::run (@command);
 }
 
 # modifies /etc/shadow directly, not good practice,
@@ -515,11 +516,11 @@ sub modify_shadow_password
 sub set_passwd
 {
   my ($login, $password) = @_;
-  my ($pwdpipe, $command);
+  my ($pwdpipe);
 
   if ($Utils::Backend::tool{"system"} eq "FreeBSD")
   {
-
+    my ($command);
     $command = "$cmd_pw usermod -H 0";
     $pwdpipe = &Utils::File::run_pipe_write ($command);
     print $pwdpipe $password;
@@ -531,10 +532,10 @@ sub set_passwd
   }
   else
   {
-    $command = "$cmd_usermod " .
-        " -p '" . $password . "' " . $login;
+    my (@command);
+    @command = ($cmd_usermod, "-p", $password, $login);
 
-    &Utils::File::run ($command);
+    &Utils::File::run (@command);
   }
 }
 
@@ -552,7 +553,7 @@ sub add_user
 
     # FreeBSD doesn't create the home directory
     $home = $$user[$HOME];
-    &Utils::File::run ("$tool_mkdir -p $home");
+    &Utils::File::run ($tool_mkdir, "-p", $home);
 
     $command = "$cmd_pw useradd " .
         " -n \'" . $$user[$LOGIN] . "\'" .
@@ -562,6 +563,13 @@ sub add_user
         " -s \'" . $$user[$SHELL] . "\'" .
         " -H 0"; # pw(8) reads password from STDIN
 
+#    @command = ($cmd_pw, "useradd", "-n", $$user[$LOGIN],
+#                                    "-u", $$user[$UID],
+#                                    "-d", $$user[$HOME],
+#                                    "-g", $$user[$GID],
+#                                    "-s", $$user[$SHELL],
+#                                    "-H", "0"); # pw(8) reads password from STDIN
+
     $pwdpipe = &Utils::File::run_pipe_write ($command);
     print $pwdpipe $$user[$PASSWD];
     &Utils::File::close_file ($pwdpipe);
@@ -570,23 +578,22 @@ sub add_user
   {
     $home_parents = $$user[$HOME];
     $home_parents =~ s/\/+[^\/]+\/*$//;
-    &Utils::File::run ("$tool_mkdir -p $home_parents");
+    &Utils::File::run ($tool_mkdir, "-p", $home_parents);
 
-    $command = "$cmd_useradd" .
-        " -d \'" . $$user[$HOME]  . "\'" .
-        " -g \'" . $$user[$GID]   . "\'" .
-        " -s \'" . $$user[$SHELL] . "\'" .
-        " -u \'" . $$user[$UID]   . "\'" .
-        " \'"    . $$user[$LOGIN] . "\'";
+    @command = ($cmd_useradd, "-d", $$user[$HOME],
+                              "-g", $$user[$GID],
+                              "-s", $$user[$SHELL],
+                              "-u", $$user[$UID],
+                                    $$user[$LOGIN]);
 
-    &Utils::File::run ($command);
+    &Utils::File::run (@command);
     &modify_shadow_password ($$user[$LOGIN], $$user[$PASSWD]);
   }
   else
   {
     $home_parents = $$user[$HOME];
     $home_parents =~ s/\/+[^\/]+\/*$//;
-    &Utils::File::run ("$tool_mkdir -p $home_parents");
+    &Utils::File::run ($tool_mkdir, "-p", $home_parents);
 
     if ($cmd_adduser &&
         $Utils::Backend::tool{"platform"} !~ /^slackware/ &&
@@ -596,34 +603,34 @@ sub add_user
     {
       # use adduser if available and valid (slackware one is b0rk)
       # set empty gecos fields and password, they will be filled out later
-      $command = "$cmd_adduser --gecos '' --disabled-password" .
-          " --home \'"  . $$user[$HOME]   . "\'" .
-          " --gid \'"   . $$user[$GID]    . "\'" .
-          " --shell \'" . $$user[$SHELL]  . "\'" .
-          " --uid \'"   . $$user[$UID]    . "\'" .
-          " \'"         . $$user[$LOGIN]  . "\'";
+      @command = ($cmd_adduser, "--gecos", "",
+                                "--disabled-password",
+                                "--home", $$user[$HOME],
+                                "--gid", $$user[$GID],
+                                "--shell", $$user[$SHELL],
+                                "--uid", $$user[$UID],
+                                         $$user[$LOGIN]);
 
-      &Utils::File::run ($command);
+      &Utils::File::run (@command);
 
       # password can't be set in non-interactive
       # mode with adduser, call usermod instead
-      $command = "$cmd_usermod " .
-          " -p '" . $$user[$PASSWD] . "' " . $$user[$LOGIN];
+      @command = ($cmd_usermod, "-p", $$user[$PASSWD], $$user[$LOGIN]);
 
-      &Utils::File::run ($command);
+      &Utils::File::run (@command);
     }
     else
     {
       # fallback to useradd
-      $command = "$cmd_useradd -m" .
-          " -d \'" . $$user[$HOME]   . "\'" .
-          " -g \'" . $$user[$GID]    . "\'" .
-          " -p \'" . $$user[$PASSWD] . "\'" .
-          " -s \'" . $$user[$SHELL]  . "\'" .
-          " -u \'" . $$user[$UID]    . "\'" .
-          " \'"    . $$user[$LOGIN]  . "\'";
-
-      &Utils::File::run ($command);
+      @command = ($cmd_useradd, "-m",
+                                "-d", $$user[$HOME],
+                                "-g", $$user[$GID],
+                                "-p", $$user[$PASSWD],
+                                "-s", $$user[$SHELL],
+                                "-u", $$user[$UID],
+                                      $$user[$LOGIN]);
+
+      &Utils::File::run (@command);
     }
   }
 
@@ -652,27 +659,24 @@ sub change_user
   }
   elsif ($Utils::Backend::tool{"system"} eq "SunOS")
   {
-    $command = "$cmd_usermod" .
-        " -d \'" . $$new_user[$HOME]   . "\'" .
-        " -g \'" . $$new_user[$GID]    . "\'" .
-        " -l \'" . $$new_user[$LOGIN]  . "\'" .
-        " -s \'" . $$new_user[$SHELL]  . "\'" .
-        " -u \'" . $$new_user[$UID]    . "\'" .
-        " \'" . $$old_user[$LOGIN] . "\'";
+    @command = ($cmd_usermod, "-d", $$new_user[$HOME],
+                              "-g", $$new_user[$GID],
+                              "-l", $$new_user[$LOGIN],
+                              "-s", $$new_user[$SHELL],
+                              "-u", $$new_user[$UID],
+                                    $$old_user[$LOGIN]);
 
-    &Utils::File::run ($command);
+    &Utils::File::run (@command);
     &modify_shadow_password ($$new_user[$LOGIN], $$new_user[$PASSWD]);
   }
   else
   {
-    $command = "$cmd_usermod" .
-        " -d \'" . $$new_user[$HOME]   . "\'" .
-        " -g \'" . $$new_user[$GID]    . "\'" .
-        " -l \'" . $$new_user[$LOGIN]  . "\'" .
-        " -p \'" . $$new_user[$PASSWD] . "\'" .
-        " -s \'" . $$new_user[$SHELL]  . "\'" .
-        " -u \'" . $$new_user[$UID]    . "\'" .
-        " \'" . $$old_user[$LOGIN] . "\'";
+    @command = ($cmd_usermod, "-d", $$new_user[$HOME],
+                              "-g", $$new_user[$GID],
+                              "-l", $$new_user[$LOGIN],
+                              "-s", $$new_user[$SHELL],
+                              "-u", $$new_user[$UID],
+                                    $$old_user[$LOGIN]);
 
     &Utils::File::run ($command);
   }