Merge branch 'danielwhite/zip-port-leak-fix'

OTP-12566

* danielwhite/zip-port-leak-fix:
  Describe zip handles an opaque type
  Document the termination semantics of a zip handle
  Prevent zip:zip_open/1,2 from leaking ports

3 files changed, 70 insertions, 20 deletions

diff --git a/lib/stdlib/doc/src/zip.xml b/lib/stdlib/doc/src/zip.xml
index 48b376743d..3845ccd09d 100644
--- a/lib/stdlib/doc/src/zip.xml
+++ b/lib/stdlib/doc/src/zip.xml
@@ -430,6 +430,8 @@
           means that subsequently reading files from the archive will be
           faster than unzipping files one at a time with <c>unzip</c>.</p>
         <p>The archive must be closed with <c>zip_close/1</c>.</p>
+        <p>The <c><anno>ZipHandle</anno></c> will be closed if the
+          process which originally opened the archive dies.</p>
       </desc>
     </func>
     <func>
diff --git a/lib/stdlib/src/zip.erl b/lib/stdlib/src/zip.erl
index b768c6d0b9..44e75ff15b 100644
--- a/lib/stdlib/src/zip.erl
+++ b/lib/stdlib/src/zip.erl
@@ -214,7 +214,9 @@
 -type zip_comment() :: #zip_comment{}.
 -type zip_file() :: #zip_file{}.
 
--export_type([create_option/0, filename/0]).
+-opaque handle() :: pid().
+
+-export_type([create_option/0, filename/0, handle/0]).
 
 %% Open a zip archive with options
 %%
@@ -500,7 +502,7 @@ do_list_dir(F, Options) ->
 
 -spec(t(Archive) -> ok when
       Archive :: file:name() | binary() | ZipHandle,
-      ZipHandle :: pid()).
+      ZipHandle :: handle()).
 
 t(F) when is_pid(F) -> zip_t(F);
 t(F) when is_record(F, openzip) -> openzip_t(F);
@@ -524,7 +526,7 @@ do_t(F, RawPrint) ->
 
 -spec(tt(Archive) -> ok when
       Archive :: file:name() | binary() | ZipHandle,
-      ZipHandle :: pid()).
+      ZipHandle :: handle()).
 
 tt(F) when is_pid(F) -> zip_tt(F);
 tt(F) when is_record(F, openzip) -> openzip_tt(F);
@@ -1114,15 +1116,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,43 +1136,47 @@ 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.
 
 -spec(zip_open(Archive) -> {ok, ZipHandle} | {error, Reason} when
       Archive :: file:name() | binary(),
-      ZipHandle :: pid(),
+      ZipHandle :: handle(),
       Reason :: term()).
 
 zip_open(Archive) -> zip_open(Archive, []).
 
 -spec(zip_open(Archive, Options) -> {ok, ZipHandle} | {error, Reason} when
       Archive :: file:name() | binary(),
-      ZipHandle :: pid(),
+      ZipHandle :: handle(),
       Options :: [Option],
       Option :: cooked | memory | {cwd, CWD :: file:filename()},
       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(),
+      ZipHandle :: handle(),
       Result :: file:name() | {file:name(), binary()},
       Reason :: term()).
 
@@ -1174,14 +1184,14 @@ zip_get(Pid) when is_pid(Pid) ->
     request(self(), Pid, get).
 
 -spec(zip_close(ZipHandle) -> ok | {error, einval} when
-      ZipHandle :: pid()).
+      ZipHandle :: handle()).
 
 zip_close(Pid) when is_pid(Pid) ->
     request(self(), Pid, close).
 
 -spec(zip_get(FileName, ZipHandle) -> {ok, Result} | {error, Reason} when
       FileName :: file:name(),
-      ZipHandle :: pid(),
+      ZipHandle :: handle(),
       Result :: file:name() | {file:name(), binary()},
       Reason :: term()).
 
@@ -1190,7 +1200,7 @@ zip_get(FileName, Pid) when is_pid(Pid) ->
 
 -spec(zip_list_dir(ZipHandle) -> {ok, Result} | {error, Reason} when
       Result :: [zip_comment() | zip_file()],
-      ZipHandle :: pid(),
+      ZipHandle :: handle(),
       Reason :: term()).
 
 zip_list_dir(Pid) when is_pid(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) ->