Merge branch 'ymaslenn/8944_set_sys_shell/OTP-19342' into maint

* ymaslenn/8944_set_sys_shell/OTP-19342:
  kernel: Use persistent term to store os_cmd_shell value
  kernel: Re-init shell when config is changed.
  Add the os_cmd_shell kernel config parameter to point to system shell for os:cmd/2

lib/kernel/doc/kernel_app.md

@@ -512,6 +512,10 @@ For more information about configuration parameters, see file
   [Escripts and non-interactive I/O in Unicode Usage in Erlang](`e:stdlib:unicode_usage.md#escripts-and-non-interactive-i-o`)
   for more details.
 
+- **`os_cmd_shell = string()`{: #os_cmd_shell }** - Specifies which shell to
+  use when invoking system commands via `os:cmd/2`. By default the shell is detected
+  automatically.
+
 ## Deprecated Configuration Parameters
 
 In Erlang/OTP 21.0, a new API for logging was added. The old `error_logger`

lib/kernel/src/kernel.erl

@@ -33,6 +33,7 @@
 start(_, []) ->
     %% Setup the logger and configure the kernel logger environment
     ok = logger:internal_init_logger(),
+    ok = os:internal_init_cmd_shell(),
     case supervisor:start_link({local, kernel_sup}, kernel, []) of
 	{ok, Pid} ->
             ok = erl_signal_handler:start(),
@@ -48,6 +49,7 @@ stop(_State) ->
 %% Some configuration parameters for kernel are changed
 %%-------------------------------------------------------------------
 config_change(Changed, New, Removed) ->
+    ok = os:internal_init_cmd_shell(),
     do_distribution_change(Changed, New, Removed),
     do_global_groups_change(Changed, New, Removed),
     ok.

lib/kernel/src/os.erl

@@ -37,6 +37,8 @@ a program to run on most platforms.
 
 -export([type/0, version/0, cmd/1, cmd/2, find_executable/1, find_executable/2]).
 
+-export([internal_init_cmd_shell/0]).
+
 -include("file.hrl").
 
 -export_type([env_var_name/0, env_var_value/0, env_var_name_value/0]).
@@ -518,6 +520,10 @@ cmd(Cmd) ->
 Executes `Command` in a command shell of the target OS, captures the standard
 output and standard error of the command, and returns this result as a string.
 
+The command shell can be set using the
+[kernel configuration parameter](kernel_app.md#os_cmd_shell), by default the
+shell is detected upon system startup.
+
 _Examples:_
 
 ```erlang
@@ -581,35 +587,16 @@ get_option(Opt, Options, Default) ->
         _ -> throw(badopt)
     end.
 
-mk_cmd({win32,Wtype}, Cmd) ->
-    Command = case {os:getenv("COMSPEC"),Wtype} of
-                  {false,windows} -> lists:concat(["command.com /c", Cmd]);
-                  {false,_} -> lists:concat(["cmd /c", Cmd]);
-                  {Cspec,_} -> lists:concat([Cspec," /c",Cmd])
-              end,
+-define(KERNEL_OS_CMD_SHELL_KEY, kernel_os_cmd_shell).
+
+mk_cmd({win32,_}, Cmd) ->
+    Shell = persistent_term:get(?KERNEL_OS_CMD_SHELL_KEY),
+    Command = lists:concat([Shell, " /c", Cmd]),
     {Command, [], [], <<>>};
 mk_cmd(_,Cmd) ->
     %% Have to send command in like this in order to make sh commands like
     %% cd and ulimit available.
-    %%
-    %% We use an absolute path here because we do not want the path to be
-    %% searched in case a stale NFS handle is somewhere in the path before
-    %% the sh command.
-    %%
-    %% Check if the default shell is located in /bin/sh as expected usually
-    %% or in /system/bin/sh as implemented on Android. The raw option is
-    %% used to bypass the file server and speed up the file access.
-    Shell = case file:read_file_info("/bin/sh",[raw]) of
-                {ok,#file_info{type=regular}} ->
-                    "/bin/sh";
-                _ ->
-                    case file:read_file_info("/system/bin/sh",[raw]) of
-                        {ok,#file_info{type=regular}} ->
-                            "/system/bin/sh";
-                        _ ->
-                            "/bin/sh"
-                    end
-            end,
+    Shell = persistent_term:get(?KERNEL_OS_CMD_SHELL_KEY),
     {Shell ++ " -s unix:cmd", [out],
      %% We insert a new line after the command, in case the command
      %% contains a comment character.
@@ -628,6 +615,42 @@ mk_cmd(_,Cmd) ->
      ["(", unicode:characters_to_binary(Cmd), "\n) </dev/null; echo \"\^D\"\n"],
      <<$\^D>>}.
 
+-doc false.
+internal_init_cmd_shell() ->
+    Shell =
+        case application:get_env(kernel, os_cmd_shell) of
+            undefined ->
+                internal_init_cmd_shell(os:type());
+            {ok, Val} ->
+                Val
+        end,
+    persistent_term:put(?KERNEL_OS_CMD_SHELL_KEY, Shell).
+internal_init_cmd_shell({win32,Wtype}) ->
+    case {os:getenv("COMSPEC"),Wtype} of
+        {false,windows} -> "command.com";
+        {false,_} -> "cmd";
+        {Cspec,_} -> Cspec
+    end;
+internal_init_cmd_shell(_) ->
+    %% We use an absolute path here because we do not want the path to be
+    %% searched in case a stale NFS handle is somewhere in the path before
+    %% the sh command.
+    %%
+    %% Check if the default shell is located in /bin/sh as expected usually
+    %% or in /system/bin/sh as implemented on Android. The raw option is
+    %% used to bypass the file server.
+    case file:read_file_info("/bin/sh",[raw]) of
+        {ok,#file_info{type=regular}} ->
+            "/bin/sh";
+        _ ->
+            case file:read_file_info("/system/bin/sh",[raw]) of
+                {ok,#file_info{type=regular}} ->
+                    "/system/bin/sh";
+                _ ->
+                    "/bin/sh"
+            end
+    end.
+
 validate(Term) ->
     try validate1(Term)
     catch error:_ -> throw(badarg)

lib/kernel/test/os_SUITE.erl

@@ -28,7 +28,7 @@
 	 find_executable/1, unix_comment_in_command/1, deep_list_command/1,
          large_output_command/1, background_command/0, background_command/1,
          message_leak/1, close_stdin/0, close_stdin/1, max_size_command/1,
-         perf_counter_api/1, error_info/1]).
+         perf_counter_api/1, error_info/1, os_cmd_shell/1,os_cmd_shell_peer/1]).
 
 -include_lib("common_test/include/ct.hrl").
 
@@ -43,7 +43,7 @@ all() ->
      find_executable, unix_comment_in_command, deep_list_command,
      large_output_command, background_command, message_leak,
      close_stdin, max_size_command, perf_counter_api,
-     error_info].
+     error_info, os_cmd_shell, os_cmd_shell_peer].
 
 groups() ->
     [].
@@ -469,6 +469,30 @@ error_info(Config) ->
         ],
     error_info_lib:test_error_info(os, L).
 
+%% Check that is *not* possible to change shell after startup
+os_cmd_shell(_Config) ->
+
+    application:set_env(kernel, os_cmd_shell, "broken shell"),
+
+    %% os:cmd should continue to work as normal
+    comp("hello", os:cmd("echo hello")).
+
+%% When started with os_cmd_shell set, we make sure that it is used.
+os_cmd_shell_peer(Config) ->
+    DataDir = proplists:get_value(data_dir, Config),
+    SysShell = "\"" ++ filename:join(DataDir, "sys_shell") ++ "\"",
+    {ok, Peer, Node} = ?CT_PEER(["-kernel","os_cmd_shell", SysShell]),
+    try erpc:call(Node, os, cmd, ["ls"], rtnode:timeout(normal)) of
+        "sys_shell" -> ok;
+        Other -> ct:fail({unexpected, Other})
+    catch
+        C:R:Stk ->
+            io:format("~p\n~p\n~p\n", [C,R,Stk]),
+            ct:fail(failed)
+    after
+        peer:stop(Peer)
+    end.
+
 no_limit_for_opened_files() ->
     case os:type() of
         {unix, freebsd} ->

lib/kernel/test/os_SUITE_data/Makefile.src

@@ -3,7 +3,7 @@ LD = @LD@
 CFLAGS = @CFLAGS@ -I@erl_include@ @DEFS@
 CROSSLDFLAGS = @CROSSLDFLAGS@
 
-PROGS = my_echo@exe@ my_fds@exe@
+PROGS = my_echo@exe@ my_fds@exe@ sys_shell@exe@
 
 all: $(PROGS)
 
@@ -18,3 +18,9 @@ my_fds@exe@: my_fds@obj@
 
 my_fds@obj@: my_fds.c
 	$(CC) -c -o my_fds@obj@ $(CFLAGS) my_fds.c
+
+sys_shell@exe@: sys_shell@obj@
+	$(LD) $(CROSSLDFLAGS) -o sys_shell sys_shell@obj@ @LIBS@
+
+sys_shell@obj@: sys_shell.c
+	$(CC) -c -o sys_shell@obj@ $(CFLAGS) sys_shell.c

lib/kernel/test/os_SUITE_data/sys_shell.c

@@ -0,0 +1,6 @@
+#include <stdio.h>
+int main(void)
+{
+    printf("sys_shell");
+    return 0;
+}