Repository navigation
Refuse companion command frames shorter than the fields they read - #3809
Open
marcelverdult wants to merge 1 commit into
Open
marcelverdult wants to merge 1 commit into
marcelverdult wants to merge 1 commit into
Conversation
marcelverdult
force-pushed
the
upstream-candidate/cmd-frame-min-len
branch
from
November 11, 2026 11:41
b58cc94 to
c3046e4
Compare
handleCmdFrame() derives every field offset from the command code. 35 of its branches test the length they need, 14 do not, or test cmd_frame[1] before the length. Neither cmd_frame nor the serial interface's receive buffer is cleared between frames, and checkRecvFrame() copies only the received bytes, so everything above len is the tail of an earlier frame. A short frame therefore acts on the previous command's bytes. No read leaves cmd_frame (the largest unconditional offset is 135, the buffer holds MAX_FRAME_SIZE + 1 bytes); the damage is acting on stale data. Twelve branches gain "&& len >= N", so a short frame falls through to the existing else and is answered like the guarded ones (ERR_CODE_UNSUPPORTED_CMD). CMD_SET_PATH_HASH_MODE and CMD_SEND_PATH_DISCOVERY_REQ only have the length test moved in front of the cmd_frame[1] test. CMD_SEND_CHANNEL_DATA gains a second check for the 2 byte data_type behind the path, whose offset is known only after path_len is parsed, like CMD_SEND_RAW_DATA. CMD_SEND_CHANNEL_DATA 3 + path + 2 CMD_ADD_UPDATE_CONTACT 136 (was 36); only lat, lon and lastmod are optional CMD_REMOVE_CONTACT 33 CMD_SHARE_CONTACT 33 CMD_GET_CONTACT_BY_KEY 33 CMD_SET_RADIO_PARAMS 11 (repeat flag stays optional) CMD_SET_RADIO_TX_POWER 2 CMD_SET_TUNING_PARAMS 9 CMD_SET_OTHER_PARAMS 2 (later fields stay optional) CMD_SET_PATH_HASH_MODE 3 CMD_REBOOT 7 CMD_FACTORY_RESET 6 CMD_SET_AUTOADD_CONFIG 2 (max hops stays optional) CMD_SEND_PATH_DISCOVERY_REQ 34 CMD_ADD_UPDATE_CONTACT is the one existing guard that changes: it asked for 36 bytes while updateContactFromFrame() reads type, flags, out_path_len, 64 bytes of path, 32 bytes of name and last_advert_timestamp without testing len. The response frame (writeContactRespFrame) writes the same fields in the same order. Measured on a T-Echo: after a long CLI frame, a short SET_TUNING_PARAMS, SET_RADIO_TX_POWER, SET_OTHER_PARAMS or SET_AUTOADD_CONFIG frame changed the setting to the stale bytes, a 1 byte GET_CONTACT_BY_KEY returned a full record, a 36 byte ADD_UPDATE_CONTACT created a contact from 100 bytes of the earlier frame, and a short REBOOT after a "reboot" CLI frame rebooted the node.
marcelverdult
force-pushed
the
upstream-candidate/cmd-frame-min-len
branch
from
November 12, 2026 12:12
c3046e4 to
1e01488
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
handleCmdFrame()derives every field offset from the command code. 35 of its branches check the length they need; 14 do not, or testcmd_frame[1]before the length.cmd_frameis not cleared between frames andcheckRecvFrame()copies only the received bytes, so everything abovelenis the tail of an earlier frame. A short frame therefore acts on the previous command's bytes.No read leaves
cmd_frame(the largest unconditional offset is 135, the buffer holdsMAX_FRAME_SIZE + 1bytes). The damage is acting on stale data. Observed on a T-Echo: after a long CLI frame, shortSET_TUNING_PARAMS,SET_RADIO_TX_POWER,SET_OTHER_PARAMSandSET_AUTOADD_CONFIGframes changed the setting to the stale bytes, a 1 byteGET_CONTACT_BY_KEYreturned a full record, a 36 byteADD_UPDATE_CONTACTcreated a contact from 100 bytes of the earlier frame, and a shortREBOOTafter arebootCLI frame rebooted the node.Fix
Twelve branches gain
&& len >= N, so a short frame falls through to the existingelseand getsERR_CODE_UNSUPPORTED_CMD, like the guarded commands.CMD_SET_PATH_HASH_MODEandCMD_SEND_PATH_DISCOVERY_REQonly have the length test moved before thecmd_frame[1]test.CMD_SEND_CHANNEL_DATAgets a second check for the 2 bytedata_typebehind the path, asCMD_SEND_RAW_DATAdoes.CMD_SEND_CHANNEL_DATACMD_ADD_UPDATE_CONTACTCMD_REMOVE_CONTACT,CMD_SHARE_CONTACT,CMD_GET_CONTACT_BY_KEYCMD_SET_RADIO_PARAMSCMD_SET_RADIO_TX_POWERCMD_SET_TUNING_PARAMSCMD_SET_OTHER_PARAMSCMD_SET_PATH_HASH_MODECMD_REBOOTCMD_FACTORY_RESETCMD_SET_AUTOADD_CONFIGCMD_SEND_PATH_DISCOVERY_REQCompatibility
CMD_ADD_UPDATE_CONTACTis the one existing guard that changes. It asked for 36 bytes, butupdateContactFromFrame()reads type, flags,out_path_len, 64 bytes of path, 32 bytes of name andlast_advert_timestamp. Only lat, lon and lastmod stay optional. The response frame (writeContactRespFrame) writes the same fields in the same order. Optional trailing fields of the other commands (repeat flag, max hops, fields aftermanual_add) stay optional.The new minimums match what KiekR, meshcore.js, meshcore_py and meshcore-open send. One exception: meshcore_py's
confirm_factory_resetsends only the command byte and no"reset", so it only ever worked when the previous frame happened to leave those bytes behind. It is now refused.