From 3adbfa4aeda9b05b03f4dcae74ddc5dfec67f178 Mon Sep 17 00:00:00 2001 From: Zhen Date: Mon, 27 Nov 2017 12:15:19 +0100 Subject: [PATCH 1/2] Close connection directly when we failed to pack a data type by not sending the corrupted data to server --- neo4j/bolt/connection.py | 7 ++++++- neo4j/v1/api.py | 3 ++- test/integration/test_session.py | 17 +++++++++++++++-- test/unit/test_packstream.py | 10 +++++----- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/neo4j/bolt/connection.py b/neo4j/bolt/connection.py index bef051054..acd0435f7 100644 --- a/neo4j/bolt/connection.py +++ b/neo4j/bolt/connection.py @@ -239,7 +239,12 @@ def append(self, signature, fields=(), response=None): log_info("C: INIT (%r, {...})", fields[0]) else: raise ValueError("Unknown message signature") - self.packer.pack_struct(signature, fields) + try: + self.packer.pack_struct(signature, fields) + except ValueError as e: + # We failed to pack this message, therefore we close this connection to avoid sending corrupted data + self.close() + raise e self.output_buffer.chunk() self.output_buffer.chunk() self.responses.append(response) diff --git a/neo4j/v1/api.py b/neo4j/v1/api.py index 4b3511926..436f28a7e 100644 --- a/neo4j/v1/api.py +++ b/neo4j/v1/api.py @@ -614,7 +614,8 @@ def rollback(self): def close(self): """ Close this transaction, triggering either a COMMIT or a ROLLBACK. """ - if not self.closed(): + conn_closed = self.session._connection.closed() + if not self.closed() and not conn_closed: try: self.sync() except CypherError: diff --git a/test/integration/test_session.py b/test/integration/test_session.py index 14b1ad73f..8e86c927e 100644 --- a/test/integration/test_session.py +++ b/test/integration/test_session.py @@ -17,7 +17,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - from unittest import SkipTest from uuid import uuid4 @@ -434,7 +433,21 @@ def test_should_sync_after_rollback(self): assert len(buffer) == 1 assert buffer[0][0] == 1 - def test_errors_on_run(self): + def test_errors_on_write_transaction(self): + session = self.driver.session() + with self.assertRaises(ValueError): + session.write_transaction(lambda tx, uuid : tx.run("CREATE (a:Thing {uuid:$uuid})", uuid=uuid), uuid4()) + session.close() + + def test_errors_on_run_transaction(self): + session = self.driver.session() + tx = session.begin_transaction() + with self.assertRaises(ValueError): + tx.run("CREATE (a:Thing {uuid:$uuid})", uuid=uuid4()) + tx.rollback() + session.close() + + def test_errors_on_run_session(self): session = self.driver.session() session.close() with self.assertRaises(SessionError): diff --git a/test/unit/test_packstream.py b/test/unit/test_packstream.py index 33f733cc7..f6ad601bb 100644 --- a/test/unit/test_packstream.py +++ b/test/unit/test_packstream.py @@ -24,6 +24,7 @@ from io import BytesIO from math import pi from unittest import TestCase +from uuid import uuid4 from neo4j.bolt.io import MessageFrame as PyMessageFrame from neo4j.packstream.packer import Packer as PyPacker @@ -273,12 +274,8 @@ def test_map_stream(self): (unpacked, unpacked_value)) def test_illegal_signature(self): - try: + with self.assertRaises(ValueError): self.assert_packable((b"XXX", ()), b"\xB0XXX") - except ValueError: - assert True - else: - assert False def test_empty_struct(self): self.assert_packable((b"X", ()), b"\xB0X") @@ -286,6 +283,9 @@ def test_empty_struct(self): def test_tiny_struct(self): self.assert_packable((b"Z", (u"A", 1)), b"\xB2Z\x81A\x01") + def test_illegal_uuid(self): + with self.assertRaises(ValueError): + self.assert_packable(uuid4(), b"\xB0XXX") try: from neo4j.bolt._io import MessageFrame as CMessageFrame From 6d8f97e007f8e379dcb8edcfd3a04445fc2b3db1 Mon Sep 17 00:00:00 2001 From: Zhen Date: Wed, 29 Nov 2017 14:08:15 +0100 Subject: [PATCH 2/2] Rebased upon parameter pre-parsing --- neo4j/bolt/connection.py | 7 +------ neo4j/v1/api.py | 3 +-- test/integration/test_session.py | 4 ++-- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/neo4j/bolt/connection.py b/neo4j/bolt/connection.py index acd0435f7..bef051054 100644 --- a/neo4j/bolt/connection.py +++ b/neo4j/bolt/connection.py @@ -239,12 +239,7 @@ def append(self, signature, fields=(), response=None): log_info("C: INIT (%r, {...})", fields[0]) else: raise ValueError("Unknown message signature") - try: - self.packer.pack_struct(signature, fields) - except ValueError as e: - # We failed to pack this message, therefore we close this connection to avoid sending corrupted data - self.close() - raise e + self.packer.pack_struct(signature, fields) self.output_buffer.chunk() self.output_buffer.chunk() self.responses.append(response) diff --git a/neo4j/v1/api.py b/neo4j/v1/api.py index 436f28a7e..4b3511926 100644 --- a/neo4j/v1/api.py +++ b/neo4j/v1/api.py @@ -614,8 +614,7 @@ def rollback(self): def close(self): """ Close this transaction, triggering either a COMMIT or a ROLLBACK. """ - conn_closed = self.session._connection.closed() - if not self.closed() and not conn_closed: + if not self.closed(): try: self.sync() except CypherError: diff --git a/test/integration/test_session.py b/test/integration/test_session.py index 8e86c927e..e17fca0f6 100644 --- a/test/integration/test_session.py +++ b/test/integration/test_session.py @@ -435,14 +435,14 @@ def test_should_sync_after_rollback(self): def test_errors_on_write_transaction(self): session = self.driver.session() - with self.assertRaises(ValueError): + with self.assertRaises(TypeError): session.write_transaction(lambda tx, uuid : tx.run("CREATE (a:Thing {uuid:$uuid})", uuid=uuid), uuid4()) session.close() def test_errors_on_run_transaction(self): session = self.driver.session() tx = session.begin_transaction() - with self.assertRaises(ValueError): + with self.assertRaises(TypeError): tx.run("CREATE (a:Thing {uuid:$uuid})", uuid=uuid4()) tx.rollback() session.close()