diff --git a/docs/api.md b/docs/api.md index 8704097..f1edd89 100644 --- a/docs/api.md +++ b/docs/api.md @@ -459,7 +459,7 @@ CoAP wire encoding/decoding (RFC 7252 + 7641 + 7959). #### `Block2Accumulator` ```python -Block2Accumulator(token, *, max_blocks=32, max_payload_bytes=65536, accepted_content_formats=None) +Block2Accumulator(token, *, max_blocks=64, max_payload_bytes=65536, accepted_content_formats=None) ``` *class*: Bounded, token-stable Block2 representation accumulator. diff --git a/smartthings_local/protocol/coap.py b/smartthings_local/protocol/coap.py index e6f8184..83f119b 100644 --- a/smartthings_local/protocol/coap.py +++ b/smartthings_local/protocol/coap.py @@ -44,11 +44,20 @@ # will honour and the only one the probes have validated end-to-end. BLOCK_SZX = 6 -# Shared bounds for callers that assemble untrusted Block2 responses. Thirty- -# two means exactly blocks 0..31; a response that advertises another block from -# block 31 is rejected. The payload cap applies to the fully assembled body. -MAX_BLOCK2_BLOCKS = 32 +# Shared bounds for callers that assemble untrusted Block2 responses. The +# payload cap applies to the fully assembled body; the block count is that same +# cap expressed in blocks of the size we ask for, so the two agree by +# construction and neither can be reached before the other. They disagreed +# until #122, where a 34-block /device/0 batch was refused by a count of 32 +# while sitting well inside 64 KiB. A count of N means exactly blocks 0..N-1; +# a response that advertises another block from block N-1 is rejected. +# +# Deriving the count ties it to BLOCK_SZX: asking for smaller blocks would +# raise it, since the bound being expressed is the assembled size rather than a +# number of round trips. A transfer that downshifts below BLOCK_SZX therefore +# stops on the payload cap only if it is large, and on this count otherwise. MAX_BLOCK2_PAYLOAD_BYTES = 64 * 1024 +MAX_BLOCK2_BLOCKS = MAX_BLOCK2_PAYLOAD_BYTES // (1 << (BLOCK_SZX + 4)) # ``classify_coap_response`` outcomes. Strings keep the helper lightweight for # transports that already use their own event/state machinery. @@ -361,7 +370,8 @@ def classify_coap_response(datagram, *, token=None, request_mid=None): class Block2Accumulator: """Bounded, token-stable Block2 representation accumulator. - At most ``max_blocks`` response blocks (32 by default) and + At most ``max_blocks`` response blocks (64 by default, which is the + default ``max_payload_bytes`` in blocks of the requested size) and ``max_payload_bytes`` assembled bytes (64 KiB by default) are accepted. Block offsets must remain contiguous. A bounded SZX downshift is accepted because the appliance firmware may return the requested payload size while diff --git a/smartthings_local/protocol/dtls_session.py b/smartthings_local/protocol/dtls_session.py index 0e36e78..713fe8e 100644 --- a/smartthings_local/protocol/dtls_session.py +++ b/smartthings_local/protocol/dtls_session.py @@ -68,7 +68,7 @@ CF_CBOR, CONTENT_FORMAT, ETAG, - MAX_BLOCK2_PAYLOAD_BYTES, + MAX_BLOCK2_BLOCKS, METHOD_DELETE, METHOD_GET, METHOD_POST, @@ -521,11 +521,10 @@ class DtlsCoapSession: HANDSHAKE_TIMEOUT_S = 12.0 READER_RECV_TIMEOUT_S = 1.0 # short so stop_event propagates quickly - # Safety bound for Block2 fetches: as many SZX-6 blocks as the shared - # MAX_BLOCK2_PAYLOAD_BYTES cap holds (64). At 32, a 33,884-byte - # /device/0?if=oic.if.b batch from an LCD_A311D_OV_QMD_EU_22K oven - # (34 blocks) failed with BlockwiseError although it was inside the cap. - MAX_BLOCKS = MAX_BLOCK2_PAYLOAD_BYTES >> (BLOCK_SZX + 4) + # Safety bound for Block2 fetches. The shared constant is the payload cap + # in blocks of the size we ask for, so this agrees with the byte cap the + # accumulator enforces beside it; #122 arrived because they did not. + MAX_BLOCKS = MAX_BLOCK2_BLOCKS def __init__(self, host, port, cert_path=None, key_path=None, *, cert_pem=None, key_pem=None, diff --git a/smartthings_local/protocol/ocf_discovery.py b/smartthings_local/protocol/ocf_discovery.py index aa26ea2..9f39caf 100644 --- a/smartthings_local/protocol/ocf_discovery.py +++ b/smartthings_local/protocol/ocf_discovery.py @@ -63,6 +63,8 @@ from .coap import ( BLOCK2_DUPLICATE, CF_CBOR, + MAX_BLOCK2_BLOCKS, + MAX_BLOCK2_PAYLOAD_BYTES, RESPONSE_MESSAGE, TYPE_CON, TYPE_NON, @@ -82,9 +84,12 @@ _DISCOVERY_PORT = 5683 _MAX_ENDPOINTS = 8 _MAX_PORTS = 8 -_MAX_BLOCKS = 32 +# Both from coap, so the plaintext directory read is bounded exactly as a +# session GET is. They were a local 32 and a local 65536 until #122 showed +# the two disagreeing on the DTLS side. +_MAX_BLOCKS = MAX_BLOCK2_BLOCKS _MAX_DATAGRAM_BYTES = 8192 -_MAX_PAYLOAD_BYTES = 65536 +_MAX_PAYLOAD_BYTES = MAX_BLOCK2_PAYLOAD_BYTES _MAX_LINKS = 256 _MAX_ENDPOINT_URIS_PER_LINK = 32 _MAX_REQUEST_OPTION_BYTES = 1024 diff --git a/tests/test_block_cap_agreement.py b/tests/test_block_cap_agreement.py new file mode 100644 index 0000000..e39e53d --- /dev/null +++ b/tests/test_block_cap_agreement.py @@ -0,0 +1,110 @@ +"""One derivation for the Block2 block count, used at every bound. + +#122 raised the session's count after a 34-block `/device/0?if=oic.if.b` +batch was refused by a count of 32 that sat well inside the 64 KiB payload +cap enforced beside it. Two more copies of that disagreement were left: the +module default every external caller gets, and the plaintext discovery read. +These pin that all three now come from one place. +""" + +from __future__ import annotations + +import secrets + +import pytest + +from smartthings_local.errors import BlockwiseError +from smartthings_local.protocol import ocf_discovery +from smartthings_local.protocol.coap import ( + BLOCK2, + BLOCK2_COMPLETE, + BLOCK_SZX, + CF_CBOR, + CONTENT_FORMAT, + MAX_BLOCK2_BLOCKS, + MAX_BLOCK2_PAYLOAD_BYTES, + TYPE_ACK, + Block2Accumulator, + block_value, + build_coap, + parse_coap_message, +) +from smartthings_local.protocol.dtls_session import DtlsCoapSession + +_BLOCK = 1 << (BLOCK_SZX + 4) + + +def test_the_block_count_is_the_payload_cap_in_blocks(): + assert MAX_BLOCK2_BLOCKS * _BLOCK == MAX_BLOCK2_PAYLOAD_BYTES + + +@pytest.mark.parametrize( + 'blocks, payload_bytes', + [ + (DtlsCoapSession.MAX_BLOCKS, MAX_BLOCK2_PAYLOAD_BYTES), + (ocf_discovery._MAX_BLOCKS, ocf_discovery._MAX_PAYLOAD_BYTES), + (MAX_BLOCK2_BLOCKS, MAX_BLOCK2_PAYLOAD_BYTES), + ], + ids=['dtls-session', 'plaintext-discovery', 'module-default'], +) +def test_every_bound_in_the_tree_agrees(blocks, payload_bytes): + # A bound that disagrees with its neighbour is the #122 defect, wherever + # it sits: the smaller one refuses transfers the larger one permits, and + # the error names the wrong cause. + assert blocks == MAX_BLOCK2_BLOCKS + assert payload_bytes == MAX_BLOCK2_PAYLOAD_BYTES + + +def _feed(accumulator, body, *, token): + """Drive one whole Block2 transfer into `accumulator`, SZX 6 throughout.""" + status = None + for number in range(-(-len(body) // _BLOCK)): + chunk = body[number * _BLOCK:(number + 1) * _BLOCK] + more = (number + 1) * _BLOCK < len(body) + status = accumulator.add_response(parse_coap_message( + build_coap( + TYPE_ACK, 0x45, 0x1234 + number, token, + ((CONTENT_FORMAT, CF_CBOR), + (BLOCK2, block_value(number, int(more), BLOCK_SZX))), + chunk, + ) + )) + return status + + +def test_the_discovery_bounds_accept_a_transfer_past_the_old_count(): + # 33 blocks is one past the count both of these carried before. The + # plaintext directory has not been measured this large on any appliance + # here, so this is the bound being asserted, not an observation. + token = secrets.token_bytes(8) + accumulator = Block2Accumulator( + token, + max_blocks=ocf_discovery._MAX_BLOCKS, + max_payload_bytes=ocf_discovery._MAX_PAYLOAD_BYTES, + accepted_content_formats={int.from_bytes(CF_CBOR, 'big')}, + ) + + body = bytes(range(256)) * (33 * _BLOCK // 256) + assert _feed(accumulator, body, token=token) == BLOCK2_COMPLETE + assert accumulator.payload == body + assert accumulator.blocks_received == 33 + + +def test_the_shared_bounds_still_refuse_the_block_after_the_last(): + # Both bounds land on the same block, so the one that fires first is + # unspecified and either error is the same answer: the body is too big. + token = secrets.token_bytes(8) + accumulator = Block2Accumulator(token) + + with pytest.raises(BlockwiseError): + _feed(accumulator, b'y' * ((MAX_BLOCK2_BLOCKS + 1) * _BLOCK), + token=token) + + +def test_a_transfer_filling_the_bounds_exactly_is_accepted(): + token = secrets.token_bytes(8) + accumulator = Block2Accumulator(token) + + body = b'z' * MAX_BLOCK2_PAYLOAD_BYTES + assert _feed(accumulator, body, token=token) == BLOCK2_COMPLETE + assert accumulator.blocks_received == MAX_BLOCK2_BLOCKS