Repository navigation
Fix decoder integer ranges, string ends and termination, and JSON bools - #79
Merged
Merged
Conversation
Binary decoder: - the 8- and 16-bit integer decoders rejected the largest value of each type (value < max instead of <=), and the signed ones had no lower bound, so -129 decoded into an int8_t as 127 - decoding a string into a char buffer accepted one exactly the size of the buffer and then wrote the terminator past its end Text decoder: - integers are range checked, negatives are rejected for unsigned types, and a token that is not a number fails instead of decoding as 0; int64_t now parses with strtoll rather than strtol - each single value (including bools, null and strings, which did not before) leaves the read position at the end of its token, so a list or map whose last value is one of these can be closed. This made functions with a bool parameter impossible to call in text mode - the end of a list or map is found after skipping whitespace - decoding a string into a char buffer now writes the terminator, and requires room for it Text encoder: bools are written as true/false rather than 1/0. Tests: re-enables the server protocol tests these fix and adds decoder tests. Corrects TextDecoder.DecodeInt64, which decoded into a uint64_t, and TextRecords.EncodeAndDecodeSimpleRecord, which decoded the whole buffer including uninitialised bytes after the encoded JSON, so it failed depending on which tests ran before it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
garethpotter
force-pushed
the
fix-decoder-ranges-and-json-bools
branch
from
November 6, 2026 13:20
f31d209 to
4b5d28e
Compare
Collaborator
|
Looks good to me! |
amitchone
approved these changes
Nov 6, 2026
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.
Fixes to the binary and text decoders found by the server protocol tests from #78, plus JSON bools in the text encoder. Re-enables the #78 tests these fix.
Binary decoder (
ThingSetBinaryDecoder.cpp)i < UINT8_MAXrather than<=(so 255, 65535, 127 and 32767 failed). The signed decoders also had no lower bound, so -129 decoded into anint8_tas 127. Both bounds are now checked.charbuffer: a string exactly as long as the buffer was accepted, then the terminator was written one byte past the end. A string must now be shorter than the buffer.Text decoder (
ThingSetTextDecoder.hpp/.cpp)256decoded into auint8_tas 0. A newdecodeInteger<T>()parses withstrtoll/strtoulland fails if the value doesn't fitT, if a negative is given for an unsigned type, or if the token isn't a number (previouslytruedecoded as 0).int64_tusedstrtol, which is 32 bits on 32-bit targets such as Zephyr. Non-integer numbers still truncate (50.6into anint32_tgives 50), as thingset-node-c does.]or}at the read position, but only the number decoders moved that position past their value. So a list ending intrue,nullor a string couldn't be closed; in practice a function with a bool parameter couldn't be called in text mode (!Exec/xVoidParams [true]gaveA0). Every single-value decode now leaves the read position at the end of its token, and the end checks skip whitespace (so[ 1 , 2 ]works) and stay within the buffer.charbuffer: no terminator was written, so the result depended on what was in the buffer before (this is whyTextDecoder.DecodeStringfailed intermittently). The terminator is now written, and the string must leave room for it.Text encoder
Bools are written as
true/falseinstead of1/0.Tests
BinaryUpdateOutOfRange,TextUpdateOutOfRange,TextExecVoidWithParams,BinaryUpdateTextGetBool, plus the maximum-value cases in the serde tables.TextDecodertests: bool literals in a list, lists ending in a string ornull, whitespace before], out-of-range and negative-into-unsigned integers, a non-number into an integer, and a string with no room for its terminator.BinaryDecodertests: a decoded string is terminated, and one with no room for the terminator is rejected without writing past the buffer.TextEncoder: the bool tests now expecttrue/false.Two existing tests were wrong:
TextDecoder.DecodeInt64decoded-9223372036854775808into auint64_tand only passed because negatives used to wrap. It now usesint64_t, as its name and expected value (INT64_MIN) intend.TextRecords.EncodeAndDecodeSimpleRecordgave the decoder the whole 1024-byte buffer, not just the encoded part, so uninitialised bytes after the JSON were parsed too. It failed depending on which tests ran before it (on macOS, in the full suite). It now passesencoder.getEncodedLength().The full suite passes: 234 tests, 10 runs out of 10, with
gccin the CI container, and 206 tests, 3 runs out of 3, on macOS. The Zephyr build is left to CI.🤖 Generated with Claude Code