From 7e3027d9bdf9dd0aaac1f6d038763f372834a233 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 19 Jul 2025 15:05:26 -1000 Subject: [PATCH 1/4] wip --- esphome/components/api/api_frame_helper.cpp | 33 +++++++++++---------- esphome/components/api/api_frame_helper.h | 3 ++ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/esphome/components/api/api_frame_helper.cpp b/esphome/components/api/api_frame_helper.cpp index afd64e8981..b7c34bc44f 100644 --- a/esphome/components/api/api_frame_helper.cpp +++ b/esphome/components/api/api_frame_helper.cpp @@ -76,6 +76,16 @@ APIError APIFrameHelper::loop() { return APIError::OK; // Convert WOULD_BLOCK to OK to avoid connection termination } +// Common socket write error handling +APIError APIFrameHelper::handle_socket_write_error_() { + if (errno == EWOULDBLOCK || errno == EAGAIN) { + return APIError::WOULD_BLOCK; + } + ESP_LOGVV(TAG, "%s: Socket write failed with errno %d", this->info_.c_str(), errno); + this->state_ = State::FAILED; + return APIError::SOCKET_WRITE_FAILED; +} + // Helper method to buffer data from IOVs void APIFrameHelper::buffer_data_from_iov_(const struct iovec *iov, int iovcnt, uint16_t total_write_len) { SendBuffer buffer; @@ -124,15 +134,13 @@ APIError APIFrameHelper::write_raw_(const struct iovec *iov, int iovcnt) { ssize_t sent = this->socket_->writev(iov, iovcnt); if (sent == -1) { - if (errno == EWOULDBLOCK || errno == EAGAIN) { + APIError err = this->handle_socket_write_error_(); + if (err == APIError::WOULD_BLOCK) { // Socket would block, buffer the data this->buffer_data_from_iov_(iov, iovcnt, total_write_len); return APIError::OK; // Success, data buffered } - // Socket error - ESP_LOGVV(TAG, "%s: Socket write failed with errno %d", this->info_.c_str(), errno); - this->state_ = State::FAILED; - return APIError::SOCKET_WRITE_FAILED; // Socket write failed + return err; // Socket write failed } else if (static_cast(sent) < total_write_len) { // Partially sent, buffer the remaining data SendBuffer buffer; @@ -173,14 +181,7 @@ APIError APIFrameHelper::try_send_tx_buf_() { ssize_t sent = this->socket_->write(front_buffer.current_data(), front_buffer.remaining()); if (sent == -1) { - if (errno != EWOULDBLOCK && errno != EAGAIN) { - // Real socket error (not just would block) - ESP_LOGVV(TAG, "%s: Socket write failed with errno %d", this->info_.c_str(), errno); - this->state_ = State::FAILED; - return APIError::SOCKET_WRITE_FAILED; // Socket write failed - } - // Socket would block, we'll try again later - return APIError::WOULD_BLOCK; + return this->handle_socket_write_error_(); } else if (sent == 0) { // Nothing sent but not an error return APIError::WOULD_BLOCK; @@ -305,12 +306,12 @@ APIError APINoiseFrameHelper::loop() { // WOULD_BLOCK when no more data is available to read while (state_ != State::DATA && this->socket_->ready()) { APIError err = state_action_(); - if (err != APIError::OK && err != APIError::WOULD_BLOCK) { - return err; - } if (err == APIError::WOULD_BLOCK) { break; } + if (err != APIError::OK) { + return err; + } } // Use base class implementation for buffer sending diff --git a/esphome/components/api/api_frame_helper.h b/esphome/components/api/api_frame_helper.h index 4bcc4acd61..9488468ba0 100644 --- a/esphome/components/api/api_frame_helper.h +++ b/esphome/components/api/api_frame_helper.h @@ -132,6 +132,9 @@ class APIFrameHelper { // Helper method to buffer data from IOVs void buffer_data_from_iov_(const struct iovec *iov, int iovcnt, uint16_t total_write_len); + + // Common socket write error handling + APIError handle_socket_write_error_(); template APIError write_raw_(const struct iovec *iov, int iovcnt, socket::Socket *socket, std::vector &tx_buf, const std::string &info, StateEnum &state, StateEnum failed_state); From 0046e677273ec264201628f3c92a2406dd759f28 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 19 Jul 2025 15:06:42 -1000 Subject: [PATCH 2/4] wip --- esphome/components/api/api_frame_helper.cpp | 35 ++++++++++----------- esphome/components/api/api_frame_helper.h | 1 + 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/esphome/components/api/api_frame_helper.cpp b/esphome/components/api/api_frame_helper.cpp index b7c34bc44f..b99c3e2614 100644 --- a/esphome/components/api/api_frame_helper.cpp +++ b/esphome/components/api/api_frame_helper.cpp @@ -299,6 +299,19 @@ APIError APINoiseFrameHelper::init() { state_ = State::CLIENT_HELLO; return APIError::OK; } +// Helper for handling handshake frame errors +APIError APINoiseFrameHelper::handle_handshake_frame_error_(APIError aerr) { + if (aerr == APIError::BAD_INDICATOR) { + send_explicit_handshake_reject_("Bad indicator byte"); + return aerr; + } + if (aerr == APIError::BAD_HANDSHAKE_PACKET_LEN) { + send_explicit_handshake_reject_("Bad handshake packet len"); + return aerr; + } + return aerr; +} + /// Run through handshake messages (if in that phase) APIError APINoiseFrameHelper::loop() { // During handshake phase, process as many actions as possible until we can't progress @@ -423,16 +436,9 @@ APIError APINoiseFrameHelper::state_action_() { // waiting for client hello ParsedFrame frame; aerr = try_read_frame_(&frame); - if (aerr == APIError::BAD_INDICATOR) { - send_explicit_handshake_reject_("Bad indicator byte"); - return aerr; + if (aerr != APIError::OK) { + return handle_handshake_frame_error_(aerr); } - if (aerr == APIError::BAD_HANDSHAKE_PACKET_LEN) { - send_explicit_handshake_reject_("Bad handshake packet len"); - return aerr; - } - if (aerr != APIError::OK) - return aerr; // ignore contents, may be used in future for flags // Reserve space for: existing prologue + 2 size bytes + frame data prologue_.reserve(prologue_.size() + 2 + frame.msg.size()); @@ -478,16 +484,9 @@ APIError APINoiseFrameHelper::state_action_() { // waiting for handshake msg ParsedFrame frame; aerr = try_read_frame_(&frame); - if (aerr == APIError::BAD_INDICATOR) { - send_explicit_handshake_reject_("Bad indicator byte"); - return aerr; + if (aerr != APIError::OK) { + return handle_handshake_frame_error_(aerr); } - if (aerr == APIError::BAD_HANDSHAKE_PACKET_LEN) { - send_explicit_handshake_reject_("Bad handshake packet len"); - return aerr; - } - if (aerr != APIError::OK) - return aerr; if (frame.msg.empty()) { send_explicit_handshake_reject_("Empty handshake message"); diff --git a/esphome/components/api/api_frame_helper.h b/esphome/components/api/api_frame_helper.h index 9488468ba0..c2b5dcf9d5 100644 --- a/esphome/components/api/api_frame_helper.h +++ b/esphome/components/api/api_frame_helper.h @@ -212,6 +212,7 @@ class APINoiseFrameHelper : public APIFrameHelper { APIError init_handshake_(); APIError check_handshake_finished_(); void send_explicit_handshake_reject_(const std::string &reason); + APIError handle_handshake_frame_error_(APIError aerr); // Pointers first (4 bytes each) NoiseHandshakeState *handshake_{nullptr}; From 722df1975805aa257d5142b76042a900a4a421fd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 19 Jul 2025 15:18:43 -1000 Subject: [PATCH 3/4] dry --- esphome/components/api/api_frame_helper.cpp | 80 ++++++++++----------- esphome/components/api/api_frame_helper.h | 1 + 2 files changed, 38 insertions(+), 43 deletions(-) diff --git a/esphome/components/api/api_frame_helper.cpp b/esphome/components/api/api_frame_helper.cpp index b99c3e2614..c0c5186932 100644 --- a/esphome/components/api/api_frame_helper.cpp +++ b/esphome/components/api/api_frame_helper.cpp @@ -312,6 +312,16 @@ APIError APINoiseFrameHelper::handle_handshake_frame_error_(APIError aerr) { return aerr; } +// Helper for handling noise library errors +APIError APINoiseFrameHelper::handle_noise_error_(int err, const char *func_name, APIError api_err) { + if (err != 0) { + state_ = State::FAILED; + HELPER_LOG("%s failed: %s", func_name, noise_err_to_str(err).c_str()); + return api_err; + } + return APIError::OK; +} + /// Run through handshake messages (if in that phase) APIError APINoiseFrameHelper::loop() { // During handshake phase, process as many actions as possible until we can't progress @@ -502,14 +512,13 @@ APIError APINoiseFrameHelper::state_action_() { noise_buffer_set_input(mbuf, frame.msg.data() + 1, frame.msg.size() - 1); err = noise_handshakestate_read_message(handshake_, &mbuf, nullptr); if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_handshakestate_read_message failed: %s", noise_err_to_str(err).c_str()); + // Special handling for MAC failure if (err == NOISE_ERROR_MAC_FAILURE) { send_explicit_handshake_reject_("Handshake MAC failure"); } else { send_explicit_handshake_reject_("Handshake error"); } - return APIError::HANDSHAKESTATE_READ_FAILED; + return handle_noise_error_(err, "noise_handshakestate_read_message", APIError::HANDSHAKESTATE_READ_FAILED); } aerr = check_handshake_finished_(); @@ -522,11 +531,10 @@ APIError APINoiseFrameHelper::state_action_() { noise_buffer_set_output(mbuf, buffer + 1, sizeof(buffer) - 1); err = noise_handshakestate_write_message(handshake_, &mbuf, nullptr); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_handshakestate_write_message failed: %s", noise_err_to_str(err).c_str()); - return APIError::HANDSHAKESTATE_WRITE_FAILED; - } + APIError aerr_write = + handle_noise_error_(err, "noise_handshakestate_write_message", APIError::HANDSHAKESTATE_WRITE_FAILED); + if (aerr_write != APIError::OK) + return aerr_write; buffer[0] = 0x00; // success aerr = write_frame_(buffer, mbuf.size + 1); @@ -584,11 +592,9 @@ APIError APINoiseFrameHelper::read_packet(ReadPacketBuffer *buffer) { noise_buffer_init(mbuf); noise_buffer_set_inout(mbuf, frame.msg.data(), frame.msg.size(), frame.msg.size()); err = noise_cipherstate_decrypt(recv_cipher_, &mbuf); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_cipherstate_decrypt failed: %s", noise_err_to_str(err).c_str()); - return APIError::CIPHERSTATE_DECRYPT_FAILED; - } + APIError decrypt_err = handle_noise_error_(err, "noise_cipherstate_decrypt", APIError::CIPHERSTATE_DECRYPT_FAILED); + if (decrypt_err != APIError::OK) + return decrypt_err; uint16_t msg_size = mbuf.size; uint8_t *msg_data = frame.msg.data(); @@ -667,11 +673,9 @@ APIError APINoiseFrameHelper::write_protobuf_packets(ProtoWriteBuffer buffer, st 4 + packet.payload_size + frame_footer_size_); int err = noise_cipherstate_encrypt(send_cipher_, &mbuf); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_cipherstate_encrypt failed: %s", noise_err_to_str(err).c_str()); - return APIError::CIPHERSTATE_ENCRYPT_FAILED; - } + APIError aerr = handle_noise_error_(err, "noise_cipherstate_encrypt", APIError::CIPHERSTATE_ENCRYPT_FAILED); + if (aerr != APIError::OK) + return aerr; // Fill in the encrypted size buf_start[1] = static_cast(mbuf.size >> 8); @@ -722,35 +726,27 @@ APIError APINoiseFrameHelper::init_handshake_() { nid_.modifier_ids[0] = NOISE_MODIFIER_PSK0; err = noise_handshakestate_new_by_id(&handshake_, &nid_, NOISE_ROLE_RESPONDER); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_handshakestate_new_by_id failed: %s", noise_err_to_str(err).c_str()); - return APIError::HANDSHAKESTATE_SETUP_FAILED; - } + APIError aerr = handle_noise_error_(err, "noise_handshakestate_new_by_id", APIError::HANDSHAKESTATE_SETUP_FAILED); + if (aerr != APIError::OK) + return aerr; const auto &psk = ctx_->get_psk(); err = noise_handshakestate_set_pre_shared_key(handshake_, psk.data(), psk.size()); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_handshakestate_set_pre_shared_key failed: %s", noise_err_to_str(err).c_str()); - return APIError::HANDSHAKESTATE_SETUP_FAILED; - } + aerr = handle_noise_error_(err, "noise_handshakestate_set_pre_shared_key", APIError::HANDSHAKESTATE_SETUP_FAILED); + if (aerr != APIError::OK) + return aerr; err = noise_handshakestate_set_prologue(handshake_, prologue_.data(), prologue_.size()); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_handshakestate_set_prologue failed: %s", noise_err_to_str(err).c_str()); - return APIError::HANDSHAKESTATE_SETUP_FAILED; - } + aerr = handle_noise_error_(err, "noise_handshakestate_set_prologue", APIError::HANDSHAKESTATE_SETUP_FAILED); + if (aerr != APIError::OK) + return aerr; // set_prologue copies it into handshakestate, so we can get rid of it now prologue_ = {}; err = noise_handshakestate_start(handshake_); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_handshakestate_start failed: %s", noise_err_to_str(err).c_str()); - return APIError::HANDSHAKESTATE_SETUP_FAILED; - } + aerr = handle_noise_error_(err, "noise_handshakestate_start", APIError::HANDSHAKESTATE_SETUP_FAILED); + if (aerr != APIError::OK) + return aerr; return APIError::OK; } @@ -766,11 +762,9 @@ APIError APINoiseFrameHelper::check_handshake_finished_() { return APIError::HANDSHAKESTATE_BAD_STATE; } int err = noise_handshakestate_split(handshake_, &send_cipher_, &recv_cipher_); - if (err != 0) { - state_ = State::FAILED; - HELPER_LOG("noise_handshakestate_split failed: %s", noise_err_to_str(err).c_str()); - return APIError::HANDSHAKESTATE_SPLIT_FAILED; - } + APIError aerr = handle_noise_error_(err, "noise_handshakestate_split", APIError::HANDSHAKESTATE_SPLIT_FAILED); + if (aerr != APIError::OK) + return aerr; frame_footer_size_ = noise_cipherstate_get_mac_length(send_cipher_); diff --git a/esphome/components/api/api_frame_helper.h b/esphome/components/api/api_frame_helper.h index c2b5dcf9d5..60ab848bee 100644 --- a/esphome/components/api/api_frame_helper.h +++ b/esphome/components/api/api_frame_helper.h @@ -213,6 +213,7 @@ class APINoiseFrameHelper : public APIFrameHelper { APIError check_handshake_finished_(); void send_explicit_handshake_reject_(const std::string &reason); APIError handle_handshake_frame_error_(APIError aerr); + APIError handle_noise_error_(int err, const char *func_name, APIError api_err); // Pointers first (4 bytes each) NoiseHandshakeState *handshake_{nullptr}; From 39050856143956e18d1905bcbdcff0a7d9888de8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 19 Jul 2025 15:22:08 -1000 Subject: [PATCH 4/4] dry --- esphome/components/api/api_frame_helper.cpp | 40 ++++++++++----------- esphome/components/api/api_frame_helper.h | 9 ++--- 2 files changed, 22 insertions(+), 27 deletions(-) diff --git a/esphome/components/api/api_frame_helper.cpp b/esphome/components/api/api_frame_helper.cpp index c0c5186932..6d4a43a9a0 100644 --- a/esphome/components/api/api_frame_helper.cpp +++ b/esphome/components/api/api_frame_helper.cpp @@ -355,7 +355,7 @@ APIError APINoiseFrameHelper::loop() { * errno API_ERROR_BAD_INDICATOR: Bad indicator byte at start of frame. * errno API_ERROR_HANDSHAKE_PACKET_LEN: Packet too big for this phase. */ -APIError APINoiseFrameHelper::try_read_frame_(ParsedFrame *frame) { +APIError APINoiseFrameHelper::try_read_frame_(std::vector *frame) { if (frame == nullptr) { HELPER_LOG("Bad argument for try_read_frame_"); return APIError::BAD_ARG; @@ -418,7 +418,7 @@ APIError APINoiseFrameHelper::try_read_frame_(ParsedFrame *frame) { #ifdef HELPER_LOG_PACKETS ESP_LOGVV(TAG, "Received frame: %s", format_hex_pretty(rx_buf_).c_str()); #endif - frame->msg = std::move(rx_buf_); + *frame = std::move(rx_buf_); // consume msg rx_buf_ = {}; rx_buf_len_ = 0; @@ -444,17 +444,17 @@ APIError APINoiseFrameHelper::state_action_() { } if (state_ == State::CLIENT_HELLO) { // waiting for client hello - ParsedFrame frame; + std::vector frame; aerr = try_read_frame_(&frame); if (aerr != APIError::OK) { return handle_handshake_frame_error_(aerr); } // ignore contents, may be used in future for flags // Reserve space for: existing prologue + 2 size bytes + frame data - prologue_.reserve(prologue_.size() + 2 + frame.msg.size()); - prologue_.push_back((uint8_t) (frame.msg.size() >> 8)); - prologue_.push_back((uint8_t) frame.msg.size()); - prologue_.insert(prologue_.end(), frame.msg.begin(), frame.msg.end()); + prologue_.reserve(prologue_.size() + 2 + frame.size()); + prologue_.push_back((uint8_t) (frame.size() >> 8)); + prologue_.push_back((uint8_t) frame.size()); + prologue_.insert(prologue_.end(), frame.begin(), frame.end()); state_ = State::SERVER_HELLO; } @@ -492,24 +492,24 @@ APIError APINoiseFrameHelper::state_action_() { int action = noise_handshakestate_get_action(handshake_); if (action == NOISE_ACTION_READ_MESSAGE) { // waiting for handshake msg - ParsedFrame frame; + std::vector frame; aerr = try_read_frame_(&frame); if (aerr != APIError::OK) { return handle_handshake_frame_error_(aerr); } - if (frame.msg.empty()) { + if (frame.empty()) { send_explicit_handshake_reject_("Empty handshake message"); return APIError::BAD_HANDSHAKE_ERROR_BYTE; - } else if (frame.msg[0] != 0x00) { - HELPER_LOG("Bad handshake error byte: %u", frame.msg[0]); + } else if (frame[0] != 0x00) { + HELPER_LOG("Bad handshake error byte: %u", frame[0]); send_explicit_handshake_reject_("Bad handshake error byte"); return APIError::BAD_HANDSHAKE_ERROR_BYTE; } NoiseBuffer mbuf; noise_buffer_init(mbuf); - noise_buffer_set_input(mbuf, frame.msg.data() + 1, frame.msg.size() - 1); + noise_buffer_set_input(mbuf, frame.data() + 1, frame.size() - 1); err = noise_handshakestate_read_message(handshake_, &mbuf, nullptr); if (err != 0) { // Special handling for MAC failure @@ -583,21 +583,21 @@ APIError APINoiseFrameHelper::read_packet(ReadPacketBuffer *buffer) { return APIError::WOULD_BLOCK; } - ParsedFrame frame; + std::vector frame; aerr = try_read_frame_(&frame); if (aerr != APIError::OK) return aerr; NoiseBuffer mbuf; noise_buffer_init(mbuf); - noise_buffer_set_inout(mbuf, frame.msg.data(), frame.msg.size(), frame.msg.size()); + noise_buffer_set_inout(mbuf, frame.data(), frame.size(), frame.size()); err = noise_cipherstate_decrypt(recv_cipher_, &mbuf); APIError decrypt_err = handle_noise_error_(err, "noise_cipherstate_decrypt", APIError::CIPHERSTATE_DECRYPT_FAILED); if (decrypt_err != APIError::OK) return decrypt_err; uint16_t msg_size = mbuf.size; - uint8_t *msg_data = frame.msg.data(); + uint8_t *msg_data = frame.data(); if (msg_size < 4) { state_ = State::FAILED; HELPER_LOG("Bad data packet: size %d too short", msg_size); @@ -612,7 +612,7 @@ APIError APINoiseFrameHelper::read_packet(ReadPacketBuffer *buffer) { return APIError::BAD_DATA_PACKET; } - buffer->container = std::move(frame.msg); + buffer->container = std::move(frame); buffer->data_offset = 4; buffer->data_len = data_len; buffer->type = type; @@ -831,7 +831,7 @@ APIError APIPlaintextFrameHelper::loop() { * * error API_ERROR_BAD_INDICATOR: Bad indicator byte at start of frame. */ -APIError APIPlaintextFrameHelper::try_read_frame_(ParsedFrame *frame) { +APIError APIPlaintextFrameHelper::try_read_frame_(std::vector *frame) { if (frame == nullptr) { HELPER_LOG("Bad argument for try_read_frame_"); return APIError::BAD_ARG; @@ -949,7 +949,7 @@ APIError APIPlaintextFrameHelper::try_read_frame_(ParsedFrame *frame) { #ifdef HELPER_LOG_PACKETS ESP_LOGVV(TAG, "Received frame: %s", format_hex_pretty(rx_buf_).c_str()); #endif - frame->msg = std::move(rx_buf_); + *frame = std::move(rx_buf_); // consume msg rx_buf_ = {}; rx_buf_len_ = 0; @@ -964,7 +964,7 @@ APIError APIPlaintextFrameHelper::read_packet(ReadPacketBuffer *buffer) { return APIError::WOULD_BLOCK; } - ParsedFrame frame; + std::vector frame; aerr = try_read_frame_(&frame); if (aerr != APIError::OK) { if (aerr == APIError::BAD_INDICATOR) { @@ -989,7 +989,7 @@ APIError APIPlaintextFrameHelper::read_packet(ReadPacketBuffer *buffer) { return aerr; } - buffer->container = std::move(frame.msg); + buffer->container = std::move(frame); buffer->data_offset = 0; buffer->data_len = rx_header_parsed_len_; buffer->type = rx_header_parsed_type_; diff --git a/esphome/components/api/api_frame_helper.h b/esphome/components/api/api_frame_helper.h index 60ab848bee..ec63cb1fd8 100644 --- a/esphome/components/api/api_frame_helper.h +++ b/esphome/components/api/api_frame_helper.h @@ -109,11 +109,6 @@ class APIFrameHelper { bool is_socket_ready() const { return socket_ != nullptr && socket_->ready(); } protected: - // Struct for holding parsed frame data - struct ParsedFrame { - std::vector msg; - }; - // Buffer containing data to be sent struct SendBuffer { std::vector data; @@ -207,7 +202,7 @@ class APINoiseFrameHelper : public APIFrameHelper { protected: APIError state_action_(); - APIError try_read_frame_(ParsedFrame *frame); + APIError try_read_frame_(std::vector *frame); APIError write_frame_(const uint8_t *data, uint16_t len); APIError init_handshake_(); APIError check_handshake_finished_(); @@ -261,7 +256,7 @@ class APIPlaintextFrameHelper : public APIFrameHelper { uint8_t frame_footer_size() override { return frame_footer_size_; } protected: - APIError try_read_frame_(ParsedFrame *frame); + APIError try_read_frame_(std::vector *frame); // Group 2-byte aligned types uint16_t rx_header_parsed_type_ = 0;