Read each request in full

read_request called recv once for the length and once for the body. A stream socket can deliver a request in several pieces, and the server then rejected that request.

The server now uses the read loop of the client, which moves to utilities as recv_exact. The server refuses a request above 64 KiB.

bmspy/client.py

@@ -7,7 +7,7 @@ import json
 import socket
 from typing import Any
 
-from bmspy.utilities import debugger
+from bmspy.utilities import debugger, recv_exact
 from bmspy.classes import UPS
 
 
@@ -45,17 +45,6 @@ def handle_registration(socket_path: str, client_name: str, debug: int = 0) -> d
     return data
 
 
-def _recv_exact(sock: socket.socket, length: int) -> bytes:
-    """Read a complete frame, even when the stream splits it across receives."""
-    data = bytearray()
-    while len(data) < length:
-        chunk = sock.recv(length - len(data))
-        if not chunk:
-            raise OSError("Daemon closed the socket before the response was complete")
-        data.extend(chunk)
-    return bytes(data)
-
-
 def socket_comms(socket_path: str, request_data: dict[str, Any], debug: int = 0) -> dict[str, Any]:
     """Send a JSON request to the daemon socket and return the decoded JSON response."""
     response_data: dict[str, Any] = dict()
@@ -101,7 +90,7 @@ def socket_comms(socket_path: str, request_data: dict[str, Any], debug: int = 0)
     # get length of expected json string
     try:
         sock.sendall(request)
-        response = _recv_exact(sock, struct.calcsize("!I"))
+        response = recv_exact(sock, struct.calcsize("!I"))
         length = struct.unpack("!I", response)[0]
         if debug > 4:
             debugger(
@@ -110,7 +99,7 @@ def socket_comms(socket_path: str, request_data: dict[str, Any], debug: int = 0)
                 )
             )
         # read length bytes
-        response = _recv_exact(sock, length)
+        response = recv_exact(sock, length)
         if debug > 3:
             debugger("socket client: incoming response: {}".format(response))
         response_data = json.loads(response)

bmspy/server.py

@@ -19,7 +19,7 @@ from dataclasses import asdict as dataclass_asdict, dataclass, field
 from types import FrameType
 from typing import Any, NoReturn
 
-from bmspy.utilities import debugger
+from bmspy.utilities import debugger, recv_exact
 from bmspy.classes import UPS
 from bmspy.jbd_bms import collect_data, initialise_serial
 from bmspy.ecoflow_bt import (
@@ -46,6 +46,8 @@ BLUETOOTH_POLL_SECONDS = 60
 FIRST_POLL_TIMEOUT = 120
 # Time limit for each read from a client and each write to it.
 CLIENT_TIMEOUT_SECONDS = 5
+# A request is a short JSON object. A larger length prefix is an error.
+MAX_REQUEST_BYTES = 64 * 1024
 
 # One adapter: collect from one Bluetooth device at a time.
 bluetooth_lock = threading.Lock()
@@ -82,7 +84,7 @@ def read_request(connection: socket.socket, debug: int = 0) -> dict[str, Any]:
     # get length of expected json string
     request = bytes()
     try:
-        request = connection.recv(struct.calcsize("!I"))
+        request = recv_exact(connection, struct.calcsize("!I"))
     except Exception as e:
         raise OSError("unable to read request length from socket: {}".format(e))
     try:
@@ -92,9 +94,14 @@ def read_request(connection: socket.socket, debug: int = 0) -> dict[str, Any]:
     if debug > 4:
         debugger("socket: incoming length: {}, encoded as {}".format(length, request))
 
+    if length > MAX_REQUEST_BYTES:
+        raise OSError(
+            "request of {} bytes exceeds the limit of {}".format(length, MAX_REQUEST_BYTES)
+        )
+
     # read length bytes
     try:
-        request = connection.recv(length)
+        request = recv_exact(connection, length)
     except Exception as e:
         raise OSError("unable to read socket: {}".format(e))
     if debug > 3:

bmspy/utilities.py

@@ -4,6 +4,7 @@
 #
 import datetime
 import pprint
+import socket
 from typing import Any
 
 
@@ -19,3 +20,16 @@ def debugger(data: Any, pretty: bool = False) -> None:
         )
     msg = f"{str(datetime.datetime.now())} {data}"
     print(msg)
+
+
+def recv_exact(sock: socket.socket, length: int) -> bytes:
+    """Read a complete frame, even when the stream splits it across receives."""
+    data = bytearray()
+    while len(data) < length:
+        chunk = sock.recv(length - len(data))
+        if not chunk:
+            raise OSError(
+                "socket closed after {} of {} bytes".format(len(data), length)
+            )
+        data.extend(chunk)
+    return bytes(data)

tests/test_server.py

@@ -253,6 +253,46 @@ class TestReadRequestErrors:
             srv.close()
             cli.close()
 
+    @pytest.mark.parametrize("split", [2, 10], ids=["inside-the-length", "inside-the-body"])
+    def test_request_in_two_pieces_is_read_whole(self, split):
+        """A stream socket can deliver one request in several pieces."""
+        srv, cli = socket_module.socketpair(socket_module.AF_UNIX, socket_module.SOCK_STREAM)
+        payload = json.dumps({"command": "GET", "client": "split"}).encode()
+        frame = struct.pack("!I", len(payload)) + payload
+        late = threading.Timer(0.05, cli.sendall, args=(frame[split:],))
+        try:
+            srv.settimeout(5)
+            cli.sendall(frame[:split])
+            late.start()
+            assert read_request(srv) == {"command": "GET", "client": "split"}
+        finally:
+            late.join()
+            srv.close()
+            cli.close()
+
+    def test_client_that_closes_after_half_a_request_is_an_error(self):
+        srv, cli = socket_module.socketpair(socket_module.AF_UNIX, socket_module.SOCK_STREAM)
+        payload = json.dumps({"command": "GET", "client": "half"}).encode()
+        try:
+            cli.sendall(struct.pack("!I", len(payload)) + payload[:10])
+            cli.close()
+            with pytest.raises(OSError, match="unable to read socket: socket closed after 10 of"):
+                read_request(srv)
+        finally:
+            srv.close()
+
+    def test_request_above_the_size_limit_is_refused(self):
+        """A wrong length prefix must not make the server wait for gigabytes."""
+        from bmspy.server import MAX_REQUEST_BYTES
+        srv, cli = socket_module.socketpair(socket_module.AF_UNIX, socket_module.SOCK_STREAM)
+        try:
+            cli.sendall(struct.pack("!I", MAX_REQUEST_BYTES + 1) + b"{}")
+            with pytest.raises(OSError, match="exceeds the limit"):
+                read_request(srv)
+        finally:
+            srv.close()
+            cli.close()
+
     def test_truncated_length_bytes_raises(self):
         srv, cli = socket_module.socketpair()
         try: