From 4d0998ef72b02d1f962b65e17311c7b66123323c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Arroyo=20Calle?= Date: Tue, 28 Sep 2021 22:48:51 +0200 Subject: [PATCH 1/5] launch exception if closing a closed TCP stream #1046 --- src/machine/streams.rs | 18 ++++++++++++++++-- src/machine/system_calls.rs | 25 ++++++++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/machine/streams.rs b/src/machine/streams.rs index e8507f13..d765d881 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -164,10 +164,10 @@ impl Drop for StreamInstance { fn drop(&mut self) { match self { StreamInstance::TcpStream(_, ref mut tcp_stream) => { - tcp_stream.shutdown(Shutdown::Both).unwrap(); + tcp_stream.shutdown(Shutdown::Both).unwrap_or(()) } StreamInstance::TlsStream(_, ref mut tls_stream) => { - tls_stream.shutdown().unwrap(); + tls_stream.shutdown().unwrap_or(()); } _ => {} } @@ -636,6 +636,20 @@ impl Stream { } } + pub(crate) fn is_closed(&self) -> bool { + match self.stream_inst.0.borrow_mut().stream_inst { + StreamInstance::Null => true, + StreamInstance::TcpStream(_, ref mut tcp_stream) => { + let mut buf = [0;8]; + match tcp_stream.peek(&mut buf) { + Ok(n_bytes) => n_bytes == 0, + Err(_) => true + } + }, + _ => false + } + } + fn unpause_stream(&mut self) { let stream_inst = match self.stream_inst.0.borrow_mut().stream_inst { StreamInstance::PausedPrologStream(ref put_back, ref mut stream_inst) diff --git a/src/machine/system_calls.rs b/src/machine/system_calls.rs index ade423a1..e14a530f 100644 --- a/src/machine/system_calls.rs +++ b/src/machine/system_calls.rs @@ -2704,10 +2704,29 @@ impl MachineState { } if !stream.is_stdin() && !stream.is_stdout() && !stream.is_stderr() { - stream.close(); + if stream.is_closed() { + let stub = MachineError::functor_stub( + clause_name!("close"), + 1, + ); - if let Some(ref alias) = stream.options().alias { - indices.stream_aliases.remove(alias); + let addr = self.heap.to_unifiable( + HeapCellValue::Stream(stream.clone()), + ); + + return Err(self.error_form( + MachineError::existence_error( + self.heap.h(), + ExistenceError::Stream(addr), + ), + stub, + )); + } else { + stream.close(); + + if let Some(ref alias) = stream.options().alias { + indices.stream_aliases.remove(alias); + } } } } From 47d811c25099906d3dcd89ea8004dc3df06ad894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Arroyo=20Calle?= Date: Thu, 30 Sep 2021 00:10:03 +0200 Subject: [PATCH 2/5] use shutdown error values to throw exceptions in close stream --- src/machine/streams.rs | 40 ++++++++++--------------------------- src/machine/system_calls.rs | 13 ++++++------ 2 files changed, 17 insertions(+), 36 deletions(-) diff --git a/src/machine/streams.rs b/src/machine/streams.rs index d765d881..e1773685 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -160,20 +160,6 @@ impl StreamInstance { } } -impl Drop for StreamInstance { - fn drop(&mut self) { - match self { - StreamInstance::TcpStream(_, ref mut tcp_stream) => { - tcp_stream.shutdown(Shutdown::Both).unwrap_or(()) - } - StreamInstance::TlsStream(_, ref mut tls_stream) => { - tls_stream.shutdown().unwrap_or(()); - } - _ => {} - } - } -} - impl fmt::Debug for StreamInstance { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { match self { @@ -596,8 +582,18 @@ impl Stream { } #[inline] - pub(crate) fn close(&mut self) { + pub(crate) fn close(&mut self) -> Result<(), std::io::Error> { + let result = match self.stream_inst.0.borrow_mut().stream_inst { + StreamInstance::TcpStream(_, ref mut tcp_stream) => { + tcp_stream.shutdown(Shutdown::Both) + }, + StreamInstance::TlsStream(_, ref mut tls_stream) => { + tls_stream.shutdown() + } + _ => Ok(()) + }; self.stream_inst.0.borrow_mut().stream_inst = StreamInstance::Null; + result } #[inline] @@ -636,20 +632,6 @@ impl Stream { } } - pub(crate) fn is_closed(&self) -> bool { - match self.stream_inst.0.borrow_mut().stream_inst { - StreamInstance::Null => true, - StreamInstance::TcpStream(_, ref mut tcp_stream) => { - let mut buf = [0;8]; - match tcp_stream.peek(&mut buf) { - Ok(n_bytes) => n_bytes == 0, - Err(_) => true - } - }, - _ => false - } - } - fn unpause_stream(&mut self) { let stream_inst = match self.stream_inst.0.borrow_mut().stream_inst { StreamInstance::PausedPrologStream(ref put_back, ref mut stream_inst) diff --git a/src/machine/system_calls.rs b/src/machine/system_calls.rs index e14a530f..cd02c750 100644 --- a/src/machine/system_calls.rs +++ b/src/machine/system_calls.rs @@ -2704,7 +2704,12 @@ impl MachineState { } if !stream.is_stdin() && !stream.is_stdout() && !stream.is_stderr() { - if stream.is_closed() { + let close_result = stream.close(); + + if let Some(ref alias) = stream.options().alias { + indices.stream_aliases.remove(alias); + } + if let Err(_) = close_result { let stub = MachineError::functor_stub( clause_name!("close"), 1, @@ -2721,12 +2726,6 @@ impl MachineState { ), stub, )); - } else { - stream.close(); - - if let Some(ref alias) = stream.options().alias { - indices.stream_aliases.remove(alias); - } } } } From 810d4c51f9b2d5e8c2729eafaa1780287bc26227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Arroyo=20Calle?= Date: Thu, 30 Sep 2021 22:59:15 +0200 Subject: [PATCH 3/5] Close Stream bug files --- tests-pl/issue1046-close-stream-client.pl | 32 +++++++++++++++++++++++ tests-pl/issue1046-close-stream.pl | 14 ++++++++++ 2 files changed, 46 insertions(+) create mode 100644 tests-pl/issue1046-close-stream-client.pl create mode 100644 tests-pl/issue1046-close-stream.pl diff --git a/tests-pl/issue1046-close-stream-client.pl b/tests-pl/issue1046-close-stream-client.pl new file mode 100644 index 00000000..db192ddc --- /dev/null +++ b/tests-pl/issue1046-close-stream-client.pl @@ -0,0 +1,32 @@ +:- use_module(library(sockets)). +:- use_module(library(time)). +:- use_module(library(format)). +:- use_module(library(charsio)). + +test :- + Addr = '0.0.0.0', + Port = 5000, + socket_client_open(Addr:Port, Stream, [type(binary)]), + read_line(Stream, Line), + write(Line), + close(Stream). + + +read_line(Stream, Line) :- + get_byte(Stream, Char), + ( Char = -1 -> + Line = [] + ; Char = 13 -> + read_line(Stream, Line) + ; Char = 10 -> + Line = [] + ; (read_line(Stream, Line0), Line = [Char|Line0]) + ). + +read_message(Stream, [Cs|Message]) :- + read_line(Stream, Bs), + chars_utf8bytes(Cs, Bs), + ( Cs = "." -> + Message = [] + ; read_message(Stream, Message) + ). diff --git a/tests-pl/issue1046-close-stream.pl b/tests-pl/issue1046-close-stream.pl new file mode 100644 index 00000000..0ac47fcc --- /dev/null +++ b/tests-pl/issue1046-close-stream.pl @@ -0,0 +1,14 @@ +:- use_module(library(sockets)). +:- use_module(library(time)). +:- use_module(library(format)). + +test :- + Addr = '0.0.0.0', + Port = 5000, + once(socket_server_open(Addr:Port, Socket)), + format("Listening at port ~d\n", [Port]), + socket_server_accept(Socket, _Client, Stream, [type(binary)]), + format(Stream, "FIRST\r\n", []), + sleep(20), + format(Stream, "SECOND\r\n", []), + close(Stream). \ No newline at end of file From 807abfef4fe1f698a5cbbea4eac6de673c2dd486 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Arroyo=20Calle?= Date: Thu, 30 Sep 2021 23:01:25 +0200 Subject: [PATCH 4/5] Close Stream bug files --- tests-pl/issue1046-close-stream.pl | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests-pl/issue1046-close-stream.pl b/tests-pl/issue1046-close-stream.pl index 0ac47fcc..63792059 100644 --- a/tests-pl/issue1046-close-stream.pl +++ b/tests-pl/issue1046-close-stream.pl @@ -2,6 +2,10 @@ :- use_module(library(time)). :- use_module(library(format)). +% Manual test to fix issue 1046 +% Server must be executed first, then client +% The expected error is an exception thrown here, not a panic + test :- Addr = '0.0.0.0', Port = 5000, From f70685d53c4cd192f8468ad7f29fb3c14b3d2d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Arroyo=20Calle?= Date: Thu, 30 Sep 2021 23:46:52 +0200 Subject: [PATCH 5/5] Close Stream test feedback --- tests-pl/issue1046-close-stream-client.pl | 22 +--------------------- tests-pl/issue1046-close-stream.pl | 2 +- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/tests-pl/issue1046-close-stream-client.pl b/tests-pl/issue1046-close-stream-client.pl index db192ddc..edfcd130 100644 --- a/tests-pl/issue1046-close-stream-client.pl +++ b/tests-pl/issue1046-close-stream-client.pl @@ -7,26 +7,6 @@ test :- Addr = '0.0.0.0', Port = 5000, socket_client_open(Addr:Port, Stream, [type(binary)]), - read_line(Stream, Line), + read_line_to_chars(Stream, Line, []), write(Line), close(Stream). - - -read_line(Stream, Line) :- - get_byte(Stream, Char), - ( Char = -1 -> - Line = [] - ; Char = 13 -> - read_line(Stream, Line) - ; Char = 10 -> - Line = [] - ; (read_line(Stream, Line0), Line = [Char|Line0]) - ). - -read_message(Stream, [Cs|Message]) :- - read_line(Stream, Bs), - chars_utf8bytes(Cs, Bs), - ( Cs = "." -> - Message = [] - ; read_message(Stream, Message) - ). diff --git a/tests-pl/issue1046-close-stream.pl b/tests-pl/issue1046-close-stream.pl index 63792059..e29202f3 100644 --- a/tests-pl/issue1046-close-stream.pl +++ b/tests-pl/issue1046-close-stream.pl @@ -9,7 +9,7 @@ test :- Addr = '0.0.0.0', Port = 5000, - once(socket_server_open(Addr:Port, Socket)), + socket_server_open(Addr:Port, Socket), format("Listening at port ~d\n", [Port]), socket_server_accept(Socket, _Client, Stream, [type(binary)]), format(Stream, "FIRST\r\n", []),