aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDaniel White <[email protected]>2015-01-17 19:20:41 +1100
committerDaniel White <[email protected]>2015-01-17 19:37:16 +1100
commitaaa7c91756da6eb4435c9a23f4725dcc1dc147aa (patch)
treea1da4bfee9a41fbb95faf01bd8064ea96c8a492b
parent905824012cef106e7bd3796bff36a2aa04b58850 (diff)
downloadotp-aaa7c91756da6eb4435c9a23f4725dcc1dc147aa.tar.gz
otp-aaa7c91756da6eb4435c9a23f4725dcc1dc147aa.tar.bz2
otp-aaa7c91756da6eb4435c9a23f4725dcc1dc147aa.zip
Prevent zip:zip_open/1,2 from leaking ports
The case was discovered where a parent process would exit before closing the zip file. The result was that a port would be left open indefinitely, as the small zip server would not detect this condition. By comparison, the file module will close the associated port when the parent exits for any reason. This change would make the zip module more consistent with the semantics of similar modules. This change is breaking for any callers expecting to pass the handle to another process for processing (assuming it exits).
-rw-r--r--lib/stdlib/src/zip.erl26
-rw-r--r--lib/stdlib/test/zip_SUITE.erl42
2 files changed, 57 insertions, 11 deletions
diff --git a/lib/stdlib/src/zip.erl b/lib/stdlib/src/zip.erl
index b768c6d0b9..cbd326ae92 100644
--- a/lib/stdlib/src/zip.erl
+++ b/lib/stdlib/src/zip.erl
@@ -1114,15 +1114,19 @@ local_file_header_from_info_method_name(#file_info{mtime = MTime},
file_name_length = length(Name),
extra_field_length = 0}.
+server_init(Parent) ->
+ %% we want to know if our parent dies
+ process_flag(trap_exit, true),
+ server_loop(Parent, not_open).
%% small, simple, stupid zip-archive server
-server_loop(OpenZip) ->
+server_loop(Parent, OpenZip) ->
receive
{From, {open, Archive, Options}} ->
case openzip_open(Archive, Options) of
{ok, NewOpenZip} ->
From ! {self(), {ok, self()}},
- server_loop(NewOpenZip);
+ server_loop(Parent, NewOpenZip);
Error ->
From ! {self(), Error}
end;
@@ -1130,19 +1134,22 @@ server_loop(OpenZip) ->
From ! {self(), openzip_close(OpenZip)};
{From, get} ->
From ! {self(), openzip_get(OpenZip)},
- server_loop(OpenZip);
+ server_loop(Parent, OpenZip);
{From, {get, FileName}} ->
From ! {self(), openzip_get(FileName, OpenZip)},
- server_loop(OpenZip);
+ server_loop(Parent, OpenZip);
{From, list_dir} ->
From ! {self(), openzip_list_dir(OpenZip)},
- server_loop(OpenZip);
+ server_loop(Parent, OpenZip);
{From, {list_dir, Opts}} ->
From ! {self(), openzip_list_dir(OpenZip, Opts)},
- server_loop(OpenZip);
+ server_loop(Parent, OpenZip);
{From, get_state} ->
From ! {self(), OpenZip},
- server_loop(OpenZip);
+ server_loop(Parent, OpenZip);
+ {'EXIT', Parent, Reason} ->
+ openzip_close(OpenZip),
+ exit({parent_died, Reason});
_ ->
{error, bad_msg}
end.
@@ -1162,8 +1169,9 @@ zip_open(Archive) -> zip_open(Archive, []).
Reason :: term()).
zip_open(Archive, Options) ->
- Pid = spawn(fun() -> server_loop(not_open) end),
- request(self(), Pid, {open, Archive, Options}).
+ Self = self(),
+ Pid = spawn_link(fun() -> server_init(Self) end),
+ request(Self, Pid, {open, Archive, Options}).
-spec(zip_get(ZipHandle) -> {ok, [Result]} | {error, Reason} when
ZipHandle :: pid(),
diff --git a/lib/stdlib/test/zip_SUITE.erl b/lib/stdlib/test/zip_SUITE.erl
index a57641ef62..d168a9d9bc 100644
--- a/lib/stdlib/test/zip_SUITE.erl
+++ b/lib/stdlib/test/zip_SUITE.erl
@@ -23,7 +23,7 @@
bad_zip/1, unzip_from_binary/1, unzip_to_binary/1,
zip_to_binary/1,
unzip_options/1, zip_options/1, list_dir_options/1, aliases/1,
- openzip_api/1, zip_api/1, unzip_jar/1,
+ openzip_api/1, zip_api/1, open_leak/1, unzip_jar/1,
compress_control/1,
foldl/1]).
@@ -38,7 +38,7 @@ all() ->
[borderline, atomic, bad_zip, unzip_from_binary,
unzip_to_binary, zip_to_binary, unzip_options,
zip_options, list_dir_options, aliases, openzip_api,
- zip_api, unzip_jar, compress_control, foldl].
+ zip_api, open_leak, unzip_jar, compress_control, foldl].
groups() ->
[].
@@ -318,8 +318,46 @@ zip_api(Config) when is_list(Config) ->
%% Clean up.
delete_files([Names]),
+ ok.
+
+open_leak(doc) ->
+ ["Test that zip doesn't leak processes and ports where the "
+ "controlling process dies without closing an zip opened with "
+ "zip:zip_open/1."];
+open_leak(suite) -> [];
+open_leak(Config) when is_list(Config) ->
+ %% Create a zip archive
+ Zip = "zip.zip",
+ {ok, Zip} = zip:zip(Zip, [], []),
+
+ %% Open archive in a another process that dies immediately.
+ ZipSrv = spawn_zip(Zip, [memory]),
+
+ %% Expect the ZipSrv process to die soon after.
+ true = spawned_zip_dead(ZipSrv),
+
+ %% Clean up.
+ delete_files([Zip]),
+
ok.
+spawn_zip(Zip, Options) ->
+ Self = self(),
+ spawn(fun() -> Self ! zip:zip_open(Zip, Options) end),
+ receive
+ {ok, ZipSrv} ->
+ ZipSrv
+ end.
+
+spawned_zip_dead(ZipSrv) ->
+ Ref = monitor(process, ZipSrv),
+ receive
+ {'DOWN', Ref, _, ZipSrv, _} ->
+ true
+ after 1000 ->
+ false
+ end.
+
unzip_options(doc) ->
["Test options for unzip, only cwd and file_list currently"];
unzip_options(suite) ->