OTP-8502 os:cmd hang

A race condition in os:cmd/1 could cause the caller to get stuck in
os:cmd/1 forever.

3 files changed, 104 insertions, 17 deletions

diff --git a/lib/kernel/doc/src/notes.xml b/lib/kernel/doc/src/notes.xml
index 7bb6aea40e..9c3f6524ae 100644
--- a/lib/kernel/doc/src/notes.xml
+++ b/lib/kernel/doc/src/notes.xml
@@ -30,6 +30,22 @@
   </header>
   <p>This document describes the changes made to the Kernel application.</p>
 
+<section><title>Kernel 2.13.5.1</title>
+
+    <section><title>Fixed Bugs and Malfunctions</title>
+      <list>
+        <item>
+          <p>
+	    A race condition in <c>os:cmd/1</c> could cause the
+	    caller to get stuck in <c>os:cmd/1</c> forever.</p>
+          <p>
+	    Own Id: OTP-8502</p>
+        </item>
+      </list>
+    </section>
+
+</section>
+
 <section><title>Kernel 2.13.5</title>
 
     <section><title>Fixed Bugs and Malfunctions</title>
diff --git a/lib/kernel/src/os.erl b/lib/kernel/src/os.erl
index 196e6cdeb2..d0b498edc9 100644
--- a/lib/kernel/src/os.erl
+++ b/lib/kernel/src/os.erl
@@ -1,19 +1,19 @@
 %%
 %% %CopyrightBegin%
-%% 
-%% Copyright Ericsson AB 1997-2009. All Rights Reserved.
-%% 
+%%
+%% Copyright Ericsson AB 1997-2010. All Rights Reserved.
+%%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
 %% compliance with the License. You should have received a copy of the
 %% Erlang Public License along with this software. If not, it can be
 %% retrieved online at http://www.erlang.org/.
-%% 
+%%
 %% Software distributed under the License is distributed on an "AS IS"
 %% basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See
 %% the License for the specific language governing rights and limitations
 %% under the License.
-%% 
+%%
 %% %CopyrightEnd%
 %%
 -module(os).
@@ -169,6 +169,7 @@ unix_cmd(Cmd) ->
 %% $1 parameter for easy identification of the resident shell.
 %%
 -define(SHELL, "/bin/sh -s unix:cmd 2>&1").
+-define(PORT_CREATOR_NAME, os_cmd_port_creator).
 
 %%
 %% Serializing open_port through a process to avoid smp lock contention
@@ -176,18 +177,37 @@ unix_cmd(Cmd) ->
 %%
 -spec start_port() -> port().
 start_port() ->
-    {Ref,Client} = {make_ref(),self()},
-    try (os_cmd_port_creator ! {Ref,Client})
-    catch
-	error:_ -> spawn(fun() -> start_port_srv({Ref,Client}) end)
-    end,
+    Ref = make_ref(),
+    Request = {Ref,self()},    
+    {Pid, Mon} = case whereis(?PORT_CREATOR_NAME) of
+		     undefined ->
+			 spawn_monitor(fun() ->
+					       start_port_srv(Request)
+				       end);
+		     P ->
+			 P ! Request,
+			 M = erlang:monitor(process, P),
+			 {P, M}
+		 end,
     receive
-	{Ref,Port} when is_port(Port) -> Port;
-	{Ref,Error} -> exit(Error)
+	{Ref, Port} when is_port(Port) ->
+	    erlang:demonitor(Mon, [flush]),
+	    Port;
+	{Ref, Error} ->
+	    erlang:demonitor(Mon, [flush]),
+	    exit(Error);
+	{'DOWN', Mon, process, Pid, _Reason} ->
+	    start_port()
     end.
 
 start_port_srv(Request) ->
-    StayAlive = try register(os_cmd_port_creator, self())
+    %% We don't want a group leader of some random application. Use
+    %% kernel_sup's group leader.
+    {group_leader, GL} = process_info(whereis(kernel_sup),
+				      group_leader),
+    true = group_leader(GL, self()),
+    process_flag(trap_exit, true),
+    StayAlive = try register(?PORT_CREATOR_NAME, self())
 		catch
 		    error:_ -> false
 		end,
@@ -196,7 +216,7 @@ start_port_srv(Request) ->
 start_port_srv_loop({Ref,Client}, StayAlive) ->
     Reply = try open_port({spawn, ?SHELL},[stream]) of
 		Port when is_port(Port) ->
-		    port_connect(Port, Client),
+		    (catch port_connect(Port, Client)),
 		    unlink(Port),
 		    Port
 	    catch
@@ -205,10 +225,19 @@ start_port_srv_loop({Ref,Client}, StayAlive) ->
 	    end,
     Client ! {Ref,Reply},
     case StayAlive of
-	true -> start_port_srv_loop(receive Msg -> Msg end, true);
+	true -> start_port_srv_loop(get_open_port_request(), true);
 	false -> exiting
     end.
 
+get_open_port_request() ->
+    receive
+	{Ref, Client} = Request when is_reference(Ref),
+				     is_pid(Client) ->
+	    Request;
+	_Junk ->
+	    get_open_port_request()
+    end.
+
 %%
 %%  unix_get_data(Port) -> Result
 %%
diff --git a/lib/kernel/test/os_SUITE.erl b/lib/kernel/test/os_SUITE.erl
index 1673b33010..6a3534b094 100644
--- a/lib/kernel/test/os_SUITE.erl
+++ b/lib/kernel/test/os_SUITE.erl
@@ -20,13 +20,13 @@
 
 -export([all/1]).
 -export([space_in_cwd/1, quoting/1, space_in_name/1, bad_command/1,
-	 find_executable/1, unix_comment_in_command/1]).
+	 find_executable/1, unix_comment_in_command/1, evil/1]).
 
 -include("test_server.hrl").
 
 all(suite) ->
     [space_in_cwd, quoting, space_in_name, bad_command, find_executable,
-     unix_comment_in_command].
+     unix_comment_in_command, evil].
 
 space_in_cwd(doc) ->
     "Test that executing a command in a current working directory "
@@ -186,6 +186,48 @@ unix_comment_in_command(Config) when is_list(Config) ->
     ?line test_server:timetrap_cancel(Dog),
     ok.
 
+-define(EVIL_PROCS, 100).
+-define(EVIL_LOOPS, 100).
+-define(PORT_CREATOR, os_cmd_port_creator).
+evil(Config) when is_list(Config) ->
+    Dog = test_server:timetrap(test_server:minutes(5)),
+    Parent = self(),
+    Ps = lists:map(fun (N) ->
+			   spawn_link(fun () ->
+					      evil_loop(Parent, ?EVIL_LOOPS,N)
+				      end)
+		   end, lists:seq(1, ?EVIL_PROCS)),
+    Devil = spawn(fun () -> devil(hd(Ps), hd(lists:reverse(Ps))) end),
+    lists:foreach(fun (P) -> receive {P, done} -> ok end end, Ps),
+    exit(Devil, kill),
+    test_server:timetrap_cancel(Dog),
+    ok.
+
+devil(P1, P2) ->
+    erlang:display({?PORT_CREATOR, whereis(?PORT_CREATOR)}),
+    (catch ?PORT_CREATOR ! lists:seq(1,1000000)),
+    (catch ?PORT_CREATOR ! lists:seq(1,666)),
+    (catch ?PORT_CREATOR ! grrrrrrrrrrrrrrrr),
+    (catch ?PORT_CREATOR ! {'EXIT', P1, buhuuu}),
+    (catch ?PORT_CREATOR ! {'EXIT', hd(erlang:ports()), buhuuu}),
+    (catch ?PORT_CREATOR ! {'EXIT', P2, arggggggg}),
+    receive after 500 -> ok end,
+    (catch exit(whereis(?PORT_CREATOR), kill)),
+    (catch ?PORT_CREATOR ! ">8|"),
+    receive after 500 -> ok end,
+    (catch exit(whereis(?PORT_CREATOR), diiiiiiiiiiiiiiiiiiiie)),
+    receive after 100 -> ok end,
+    devil(P1, P2).
+
+evil_loop(Parent, Loops, N) ->
+    Res = integer_to_list(N),
+    evil_loop(Parent, Loops, Res, "echo " ++ Res).
+
+evil_loop(Parent, 0, _Res, _Cmd) ->
+    Parent ! {self(), done};
+evil_loop(Parent, Loops, Res, Cmd) ->
+    comp(Res, os:cmd(Cmd)),
+    evil_loop(Parent, Loops-1, Res, Cmd).
 
 comp(Expected, Got) ->
     case strip_nl(Got) of