Merge branch 'lukas/erts/fix_outputv_port_task_cleanup/ERL-428/OTP-14481' into maint-20

* lukas/erts/fix_outputv_port_task_cleanup/ERL-428/OTP-14481:
  fixup! erts: Cleanup dropped port tasks correctly
  erts: Add tests to detect port close race
  erts: Cleanup dropped port tasks correctly

4 files changed, 113 insertions, 57 deletions

diff --git a/erts/emulator/beam/erl_port_task.c b/erts/emulator/beam/erl_port_task.c
index a044de3fee..1ab1e47254 100644
--- a/erts/emulator/beam/erl_port_task.c
+++ b/erts/emulator/beam/erl_port_task.c
@@ -852,10 +852,11 @@ schedule_port_task_handle_list_free(ErtsPortTaskHandleList *pthlp)
 }
 
 static ERTS_INLINE void
-abort_nosuspend_task(Port *pp,
-		     ErtsPortTaskType type,
-		     ErtsPortTaskTypeData *tdp,
-		     int bpq_data)
+abort_signal_task(Port *pp,
+                  int abort_type,
+                  ErtsPortTaskType type,
+                  ErtsPortTaskTypeData *tdp,
+                  int bpq_data)
 {
 
     ASSERT(type == ERTS_PORT_TASK_PROC_SIG);
@@ -863,18 +864,28 @@ abort_nosuspend_task(Port *pp,
     if (!bpq_data)
 	tdp->psig.callback(NULL,
 			   ERTS_PORT_SFLG_INVALID,
-			   ERTS_PROC2PORT_SIG_ABORT_NOSUSPEND,
+			   abort_type,
 			   &tdp->psig.data);
     else {
 	ErlDrvSizeT size = erts_proc2port_sig_command_data_size(&tdp->psig.data);
 	tdp->psig.callback(NULL,
 			   ERTS_PORT_SFLG_INVALID,
-			   ERTS_PROC2PORT_SIG_ABORT_NOSUSPEND,
+			   abort_type,
 			   &tdp->psig.data);
 	aborted_proc2port_data(pp, size);
     }
 }
 
+
+static ERTS_INLINE void
+abort_nosuspend_task(Port *pp,
+		     ErtsPortTaskType type,
+		     ErtsPortTaskTypeData *tdp,
+		     int bpq_data)
+{
+    abort_signal_task(pp, ERTS_PROC2PORT_SIG_ABORT_NOSUSPEND, type, tdp, bpq_data);
+}
+
 static ErtsPortTaskHandleList *
 get_free_nosuspend_handles(Port *pp)
 {
@@ -1613,8 +1624,9 @@ abort_nosuspend:
 
     ASSERT(ns_pthlp);
     erts_free(ERTS_ALC_T_PT_HNDL_LIST, ns_pthlp);
-    if (ptp)
-	port_task_free(ptp);
+
+    ASSERT(ptp);
+    port_task_free(ptp);
 
     return 0;
 
@@ -1625,12 +1637,15 @@ fail:
 	erts_port_dec_refc(pp);
 #endif
 
+    if (ptp) {
+        abort_signal_task(pp, ERTS_PROC2PORT_SIG_ABORT,
+                          ptp->type, &ptp->u.alive.td, 0);
+	port_task_free(ptp);
+    }
+
     if (ns_pthlp)
 	erts_free(ERTS_ALC_T_PT_HNDL_LIST, ns_pthlp);
 
-    if (ptp)
-	port_task_free(ptp);
-
     return -1;
 }
 
diff --git a/erts/emulator/beam/io.c b/erts/emulator/beam/io.c
index d25e53ada0..75545df80a 100644
--- a/erts/emulator/beam/io.c
+++ b/erts/emulator/beam/io.c
@@ -1968,7 +1968,6 @@ int
 erts_port_output_async(Port *prt, Eterm from, Eterm list)
 {
 
-    ErtsPortOpResult res;
     ErtsProc2PortSigData *sigdp;
     erts_driver_t *drv = prt->drv_ptr;
     size_t size;
@@ -2102,26 +2101,18 @@ erts_port_output_async(Port *prt, Eterm from, Eterm list)
 	sigdp->u.output.size = size;
 	port_sig_callback = port_sig_output;
     }
-    sigdp->flags = 0;
     ns_pthp = NULL;
     task_flags = 0;
 
-    res = erts_schedule_proc2port_signal(NULL,
-					 prt,
-					 ERTS_INVALID_PID,
-					 NULL,
-					 sigdp,
-					 task_flags,
-					 ns_pthp,
-					 port_sig_callback);
+    erts_schedule_proc2port_signal(NULL,
+                                   prt,
+                                   ERTS_INVALID_PID,
+                                   NULL,
+                                   sigdp,
+                                   task_flags,
+                                   ns_pthp,
+                                   port_sig_callback);
 
-    if (res != ERTS_PORT_OP_SCHEDULED) {
-	if (drv->outputv)
-	    cleanup_scheduled_outputv(evp, cbin);
-	else
-	    cleanup_scheduled_output(buf);
-	return 1;
-    }
     return 1;
 
 bad_value:
@@ -2554,10 +2545,6 @@ erts_port_output(Process *c_p,
 					 port_sig_callback);
 
     if (res != ERTS_PORT_OP_SCHEDULED) {
-	if (drv->outputv)
-	    cleanup_scheduled_outputv(evp, cbin);
-	else
-	    cleanup_scheduled_output(buf);
 	return res;
     }
 
@@ -2736,21 +2723,14 @@ erts_port_exit(Process *c_p,
 					   &bp->off_heap);
     }
 
-    res = erts_schedule_proc2port_signal(c_p,
-					 prt,
-					 c_p ? c_p->common.id : from,
-					 refp,
-					 sigdp,
-					 0,
-					 NULL,
-					 port_sig_exit);
-
-    if (res == ERTS_PORT_OP_DROPPED) {
-	if (bp)
-	    free_message_buffer(bp);
-    }
-
-    return res;
+    return erts_schedule_proc2port_signal(c_p,
+                                          prt,
+                                          c_p ? c_p->common.id : from,
+                                          refp,
+                                          sigdp,
+                                          0,
+                                          NULL,
+                                          port_sig_exit);
 }
 
 static ErtsPortOpResult
@@ -4930,10 +4910,9 @@ erts_port_control(Process* c_p,
 					 0,
 					 NULL,
 					 port_sig_control);
-    if (res != ERTS_PORT_OP_SCHEDULED) {
-	cleanup_scheduled_control(binp, bufp);
+    if (res != ERTS_PORT_OP_SCHEDULED)
 	return ERTS_PORT_OP_BADARG;
-    }
+
     return res;
 }
 
@@ -5223,10 +5202,9 @@ erts_port_call(Process* c_p,
 					 0,
 					 NULL,
 					 port_sig_call);
-    if (res != ERTS_PORT_OP_SCHEDULED) {
-	cleanup_scheduled_call(bufp);
+    if (res != ERTS_PORT_OP_SCHEDULED)
 	return ERTS_PORT_OP_BADARG;
-    }
+
     return res;
 }
 
diff --git a/erts/emulator/test/port_SUITE.erl b/erts/emulator/test/port_SUITE.erl
index f512fa3a57..ab0b1a82bd 100644
--- a/erts/emulator/test/port_SUITE.erl
+++ b/erts/emulator/test/port_SUITE.erl
@@ -86,6 +86,7 @@
     cd_relative/1,
     close_deaf_port/1,
     count_fds/1,
+    dropped_commands/1,
     dying_port/1,
     env/1,
     eof/1,
@@ -548,6 +549,45 @@ make_dying_port(Config) when is_list(Config) ->
     Command = lists:concat([PortTest, " -h0 -d -q"]),
     open_port({spawn, Command}, [stream]).
 
+%% Test that dropped port_commands work correctly.
+%% This used to cause a segfault.
+%%
+%% This testcase creates a port and then lets many processes
+%% do parallel commands to it. After a while it closes the
+%% port and we are trying to catch the race when doing a
+%% command while the port is closing.
+dropped_commands(Config) ->
+    %% Test with output callback
+    dropped_commands(Config, false, {self(), {command, "1"}}),
+    %% Test with outputv callback
+    dropped_commands(Config, true, {self(), {command, "1"}}).
+
+dropped_commands(Config, Outputv, Cmd) ->
+    Path = proplists:get_value(data_dir, Config),
+    os:putenv("ECHO_DRV_USE_OUTPUTV", atom_to_list(Outputv)),
+    ok = load_driver(Path, "echo_drv"),
+    [dropped_commands_test(Cmd) || _ <- lists:seq(1, 100)],
+    timer:sleep(100),
+    erl_ddll:unload_driver("echo_drv"),
+    ok.
+
+dropped_commands_test(Cmd) ->
+    Port = erlang:open_port({spawn_driver, "echo_drv"}, [{parallelism, true}]),
+    spawn_monitor(
+      fun() ->
+              [spawn_link(fun() -> spin(Port, Cmd) end) || _ <- lists:seq(1,8)],
+              timer:sleep(5),
+              port_close(Port),
+              timer:sleep(5),
+              exit(nok)
+      end),
+    receive _M -> timer:sleep(5) end.
+
+spin(P, Cmd) ->
+    P ! Cmd,
+    spin(P, Cmd).
+
+
 %% Tests that port program with complete path (but without any
 %% .exe extension) can be started, even if there is a file with
 %% the same name but without the extension in the same directory.
diff --git a/erts/emulator/test/port_SUITE_data/echo_drv.c b/erts/emulator/test/port_SUITE_data/echo_drv.c
index 1d39c6a00c..b4370f6455 100644
--- a/erts/emulator/test/port_SUITE_data/echo_drv.c
+++ b/erts/emulator/test/port_SUITE_data/echo_drv.c
@@ -18,8 +18,11 @@ typedef struct _erl_drv_data EchoDrvData;
 
 static EchoDrvData *echo_drv_start(ErlDrvPort port, char *command);
 static void         echo_drv_stop(EchoDrvData *data_p);
-static void         echo_drv_output(ErlDrvData drv_data, char *buf,
-				    ErlDrvSizeT len);
+static void         echo_drv_output(ErlDrvData drv_data, char *buf, ErlDrvSizeT len);
+static ErlDrvSSizeT echo_control(ErlDrvData drv_data,
+                                 unsigned int command, char *buf,
+                                 ErlDrvSizeT len, char **rbuf, ErlDrvSizeT rlen);
+static void         echo_outputv(ErlDrvData drv_data, ErlIOVec *ev);
 static void         echo_drv_finish(void);
 
 static ErlDrvEntry echo_drv_entry = { 
@@ -32,9 +35,9 @@ static ErlDrvEntry echo_drv_entry = {
     "echo_drv",
     echo_drv_finish,
     NULL, /* handle */
-    NULL, /* control */
+    echo_control, /* control */
     NULL, /* timeout */
-    NULL, /* outputv */
+    echo_outputv, /* outputv */
     NULL, /* ready_async */
     NULL,
     NULL,
@@ -56,6 +59,14 @@ static ErlDrvEntry echo_drv_entry = {
 
 DRIVER_INIT(echo_drv)
 {
+    char buf[10];
+    size_t bufsz = sizeof(buf);
+    char *use_outputv;
+    use_outputv = (erl_drv_getenv("ECHO_DRV_USE_OUTPUTV", buf, &bufsz) == 0
+		   ? buf
+		   : "false");
+    if (strcmp(use_outputv, "true") != 0)
+	echo_drv_entry.outputv = NULL;
     return &echo_drv_entry;
 }
 
@@ -87,3 +98,15 @@ static void echo_drv_output(ErlDrvData drv_data, char *buf, ErlDrvSizeT len) {
 
 static void echo_drv_finish() {
 }
+
+static ErlDrvSSizeT echo_control(ErlDrvData drv_data,
+                                 unsigned int command, char *buf,
+                                 ErlDrvSizeT len, char **rbuf, ErlDrvSizeT rlen)
+{
+    return 0;
+}
+
+static void echo_outputv(ErlDrvData drv_data, ErlIOVec *ev)
+{
+    return;
+}