From c4e1360cdf5ee6e2b4e6f65cc143ccafe6dd8bf7 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 7 Sep 2026 15:57:10 +0200 Subject: [PATCH] [api] Check the encoded end against the reserved size under ESPHOME_DEBUG_API The fixed32 store helper moves to a private section since it neither bounds checks nor advances the cursor, its comment describes the path each target takes, the generated file scan flags any ProtoEncode call that does not assign the cursor, and StateWaiter timeouts can carry a label so gathered waits are told apart. --- esphome/components/api/api_connection.cpp | 7 ++++- .../components/api/api_connection_buffer.h | 8 ++++- esphome/components/api/proto.h | 30 +++++++++++-------- tests/integration/state_utils.py | 7 +++-- .../integration/test_api_encode_boundaries.py | 6 ++-- .../components/api/test_api_proto.py | 2 +- 6 files changed, 40 insertions(+), 20 deletions(-) diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index da4b7d7702..d104aa81c9 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -2255,7 +2255,12 @@ bool APIConnection::send_message_(uint32_t payload_size, uint16_t message_type, // Capacity reserved above, cannot fail (void) shared_buf.resize(write_start + payload_size); ProtoWriteBuffer buffer{&shared_buf, write_start}; - encode_fn(msg, buffer PROTO_ENCODE_DEBUG_INIT(&shared_buf)); + uint8_t *end = encode_fn(msg, buffer PROTO_ENCODE_DEBUG_INIT(&shared_buf)); +#ifdef ESPHOME_DEBUG_API + assert(end == shared_buf.data() + shared_buf.size()); +#else + (void) end; +#endif return this->send_buffer(ProtoWriteBuffer{&shared_buf}, message_type); } // encode_to_buffer is defined inline in api_connection.h (ESPHOME_ALWAYS_INLINE) diff --git a/esphome/components/api/api_connection_buffer.h b/esphome/components/api/api_connection_buffer.h index 08520249bf..1f3400a88e 100644 --- a/esphome/components/api/api_connection_buffer.h +++ b/esphome/components/api/api_connection_buffer.h @@ -46,7 +46,13 @@ inline uint16_t ESPHOME_ALWAYS_INLINE APIConnection::encode_to_buffer(uint32_t c return 0; } ProtoWriteBuffer buffer{&shared_buf, shared_buf.size() - calculated_size}; - encode_fn(msg, buffer PROTO_ENCODE_DEBUG_INIT(&shared_buf)); + uint8_t *end = encode_fn(msg, buffer PROTO_ENCODE_DEBUG_INIT(&shared_buf)); +#ifdef ESPHOME_DEBUG_API + // A body that writes fewer bytes than calculate_size() promised would ship stale buffer bytes + assert(end == shared_buf.data() + shared_buf.size()); +#else + (void) end; +#endif return total_calculated_size; } diff --git a/esphome/components/api/proto.h b/esphome/components/api/proto.h index c29f46f0b4..0d6b455a0e 100644 --- a/esphome/components/api/proto.h +++ b/esphome/components/api/proto.h @@ -418,19 +418,6 @@ class ProtoEncode { std::memcpy(pos + 2, ref.c_str(), ref.size()); return pos + 2 + ref.size(); } - /// Unaligned little-endian store; __builtin_memcpy stays inline even under -fno-builtin-memcpy. - static inline void ESPHOME_ALWAYS_INLINE write_fixed32_le(uint8_t *__restrict__ pos, uint32_t value) { - if constexpr (PROTO_FIXED32_BYTE_STORES) { - // Spelled out so the outlined helper does not itself become a memcpy call - pos[0] = static_cast(value); - pos[1] = static_cast(value >> 8); - pos[2] = static_cast(value >> 16); - pos[3] = static_cast(value >> 24); - } else { - const uint32_t le = convert_little_endian(value); - __builtin_memcpy(pos, &le, 4); - } - } /// Write a precomputed tag byte + 32-bit value. Outlined on embedded: one copy beats inline stores per field. [[nodiscard]] static PROTO_OUTLINE_FOR_SIZE uint8_t *write_tag_and_fixed32( uint8_t *__restrict__ pos PROTO_ENCODE_DEBUG_PARAM, uint8_t tag, uint32_t value) { @@ -594,6 +581,23 @@ class ProtoEncode { buffer.encode_optional_sub_message(field_id, value); return buffer.get_pos(); } + + private: + /// Unaligned little endian store of four bytes: byte stores where the outlined helper lives (ESP-IDF, ARM + /// without unaligned access), otherwise a memcpy the compiler folds into one store. Callers bounds check + /// and advance the cursor themselves. + static inline void ESPHOME_ALWAYS_INLINE write_fixed32_le(uint8_t *__restrict__ pos, uint32_t value) { + if constexpr (PROTO_FIXED32_BYTE_STORES) { + // Spelled out so the outlined helper does not itself become a memcpy call + pos[0] = static_cast(value); + pos[1] = static_cast(value >> 8); + pos[2] = static_cast(value >> 16); + pos[3] = static_cast(value >> 24); + } else { + const uint32_t le = convert_little_endian(value); + __builtin_memcpy(pos, &le, 4); + } + } }; #undef PROTO_OUTLINE_FOR_SIZE #undef PROTO_FIXED32_BYTE_STORES diff --git a/tests/integration/state_utils.py b/tests/integration/state_utils.py index 8611f700db..0e354f835a 100644 --- a/tests/integration/state_utils.py +++ b/tests/integration/state_utils.py @@ -78,7 +78,10 @@ class StateWaiter: future.set_result(state) async def expect( - self, predicate: Callable[[EntityState], bool], timeout: float = 5.0 + self, + predicate: Callable[[EntityState], bool], + timeout: float = 5.0, + label: str | None = None, ) -> EntityState: """Wait for the next state matching ``predicate``; states seen before this call do not count.""" entry = (predicate, asyncio.get_running_loop().create_future()) @@ -88,7 +91,7 @@ class StateWaiter: return await entry[1] except TimeoutError: raise TimeoutError( - f"no state matched {predicate} within {timeout}s" + f"no state matched {label or predicate} within {timeout}s" ) from None finally: self._waiters.remove(entry) diff --git a/tests/integration/test_api_encode_boundaries.py b/tests/integration/test_api_encode_boundaries.py index 6ab7bc7476..7fac620481 100644 --- a/tests/integration/test_api_encode_boundaries.py +++ b/tests/integration/test_api_encode_boundaries.py @@ -64,13 +64,15 @@ async def test_api_encode_boundaries( isinstance(s, SensorState) and s.key == sensor.key and s.state == 12.5 - ) + ), + label="sensor 12.5", ), waiter.expect( lambda s: ( isinstance(s, TextSensorState) and s.key == text.key and s.state == "y" * 200 - ) + ), + label="text 200 x y", ), ) diff --git a/tests/unit_tests/components/api/test_api_proto.py b/tests/unit_tests/components/api/test_api_proto.py index b83f49c612..a194511f2b 100644 --- a/tests/unit_tests/components/api/test_api_proto.py +++ b/tests/unit_tests/components/api/test_api_proto.py @@ -387,6 +387,6 @@ def test_generated_encode_calls_keep_the_cursor() -> None: dropped = [ line for line in CPP_TEXT.splitlines() - if line.lstrip().startswith("ProtoEncode::") + if "ProtoEncode::" in line and "pos = ProtoEncode::" not in line ] assert not dropped, dropped[:5]