From ade99a7081bc00c536f5717e9acebcc4b3082a45 Mon Sep 17 00:00:00 2001 From: Rickard Green Date: Wed, 10 Mar 2010 09:48:27 +0000 Subject: OTP-8502 os:cmd hang A race condition in os:cmd/1 could cause the caller to get stuck in os:cmd/1 forever. --- lib/kernel/doc/src/notes.xml | 16 ++++++++++++ lib/kernel/src/os.erl | 59 +++++++++++++++++++++++++++++++++----------- lib/kernel/test/os_SUITE.erl | 46 ++++++++++++++++++++++++++++++++-- 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 @@

This document describes the changes made to the Kernel application.

+
Kernel 2.13.5.1 + +
Fixed Bugs and Malfunctions + + +

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

+

+ Own Id: OTP-8502

+ + +
+ +
+
Kernel 2.13.5
Fixed Bugs and Malfunctions 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 -- cgit v1.2.3