From b8d8ece734a06a8d6cb40e6cdafed6399b2edf31 Mon Sep 17 00:00:00 2001 From: Jack Nagy Date: Sat, 10 Oct 2026 15:42:15 +0100 Subject: [PATCH] fix(protocol): derive the Block2 block count from the payload cap, once #122 raised `DtlsCoapSession.MAX_BLOCKS` after a 34-block `/device/0?if=oic.if.b` batch on an LCD_A311D_OV_QMD_EU_22K oven was refused by a count of 32 while sitting well inside the 64 KiB payload cap the same accumulator enforces beside it. Mandevilla57 measured that and scoped the fix to the session, which was right: no measurement anywhere here approaches the other two copies. Those copies are the same disagreement. `coap.MAX_BLOCK2_BLOCKS` is the default every caller outside this package gets, and `ocf_discovery` carried a local 32 against a local 65536 for the plaintext directory read. So the count is now derived from the payload cap in `coap` and both bounds point at it. Deriving it also says which bound is the real one: the assembled size. The count is that size in blocks of the size we ask for, so neither can be reached before the other, and asking for smaller blocks raises the count rather than shrinking what a caller may read. The session keeps #122's behaviour exactly, through the shared name instead of its own arithmetic. tests/test_block_cap_agreement.py pins all three bounds against one derivation and drives a 33-block transfer through the discovery bounds, one past what they allowed. Four of its seven fail on 4336f13. The plaintext directory has not been measured past 32 blocks on any appliance here, so that test asserts the bound and not an observation. docs/api.md regenerated: the published `Block2Accumulator` signature now shows max_blocks=64. --- docs/api.md | 2 +- smartthings_local/protocol/coap.py | 20 +++- smartthings_local/protocol/dtls_session.py | 11 +- smartthings_local/protocol/ocf_discovery.py | 9 +- tests/test_block_cap_agreement.py | 110 ++++++++++++++++++++ 5 files changed, 138 insertions(+), 14 deletions(-) create mode 100644 tests/test_block_cap_agreement.py 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