From 9b47b95b0f93d20d2247cdc6baab3229652db64c Mon Sep 17 00:00:00 2001 From: Ray Ozzie Date: Tue, 10 Feb 2026 09:22:10 -0500 Subject: [PATCH] Fix COBS code-byte count: ceil(u/254) vs floor(u/254) + 1 ====================================================================== Background ---------- The Notecard firmware's cobsGuaranteedFit() tells the host library the maximum number of unencoded binary bytes that will safely fit in the Notecard's I/O buffer after COBS encoding. For years, the Notecard used a simple, slightly conservative formula: overhead = 1 + (buflen / 254) + 1 return buflen - overhead This estimates the COBS overhead from the *buffer* size rather than from the (smaller) payload size, which systematically overestimates the number of code bytes by a small amount. For the Notecard's 256 KB I/O buffer (262,144 bytes), this formula reports max = 261,110. A recent firmware optimization replaced this with a formula intended to compute the mathematically exact answer. For the same buffer it now reports max = 261,114 -- four bytes more than before. A customer immediately reported that filling the buffer to this new maximum causes card.binary.get to fail. The old limit of 261,110 had always worked; 261,114 does not. The bug: ceil(u/254) vs floor(u/254) + 1 ----------------------------------------- COBS encoding partitions data into blocks of up to 254 bytes, prefixing each block with a "code byte" that indicates its length (or the distance to the next zero). The central question for buffer-size arithmetic is: how many code bytes does worst-case encoding produce for u data bytes? The note-c library computed this as: ceil(u / 254) The correct answer is: floor(u / 254) + 1 For most values these are identical. They diverge at exact multiples of 254: ceil(254 / 254) = 1 WRONG floor(254 / 254) + 1 = 2 CORRECT Why the +1? Walk through what the encoder actually does with 254 non-zero bytes: 1. Reserve initial code byte (code = 1) 2. Copy 254 data bytes (code increments to 0xFF) 3. code == 0xFF: write it, reserve new (first code byte emitted) 4. No data remains; loop exits 5. Write final code byte = 0x01 (second code byte emitted) Output: [0xFF] [254 data bytes] [0x01] = 256 bytes The encoder ALWAYS writes a final code byte for the last block, even when the data ends exactly on a 254-byte boundary. That final code byte is the one that ceil() misses. ceil(254/254) = 1, but the encoder emitted two code bytes. This error appeared in two note-c functions: _cobsEncodedMaxLength(u): Used ceil(u/254) for the code byte count, underestimating by 1 at exact multiples of 254. For u = 254 it returned 256 instead of the correct 257. _cobsGuaranteedFit(bufLen): The algebraic inverse, which solved for the largest payload that fits in a given buffer. It used the same flawed ceil-based derivation. At buffer sizes of the form 255k + 1 (i.e. 256, 511, 766, ...) it overestimated the safe payload by 1, promising a fit that would actually overflow after encoding. Why it was hidden, and why it surfaced -------------------------------------- The Notecard's old conservative cobsGuaranteedFit reported max = 261,110 for a 262,144-byte buffer. The exact answer is 261,114. That four-byte gap meant the host never attempted to store a payload at a size where the ceil-vs-floor discrepancy in _cobsEncodedMaxLength could matter. The conservative formula had been quietly absorbing the note-c library's arithmetic error for years. When the Notecard's formula was updated to report the exact maximum, the safety margin vanished. The host was now operating at the true edge of the buffer, where every byte of COBS overhead must be accounted for precisely. Any arithmetic error -- in the host library's buffer validation, in the encoding expansion calculation, or anywhere else in the pipeline -- would now manifest as a real failure instead of being silently absorbed by the four spare bytes. The Notecard firmware has been reverted to the old conservative formula. It has a long track record, is obviously correct (it can only underestimate capacity, never overestimate), and its cost is negligible: roughly bufLen/64770 bytes of unused capacity, which is about 4 bytes per 256 KB. The fix (note-c) ---------------- _cobsEncodedMaxLength: Replace the ceil-based code byte computation with floor + 1: Old: codeBytes = (length / 254) + ((length % 254) > 0) // ceil New: codeBytes = (length / 254) + 1 // floor + 1 _cobsGuaranteedFit: Replace the flawed inverse derivation. Given the constraint u + floor(u/254) + 2 <= bufLen: Old: e = bufLen - 1; return e - floor((e + 254) / 255) New: t = bufLen - 2; return t - floor((t + 1) / 255) Both new formulas have been verified against a reference COBS encoder for all tested values from 0 to 262,144. Tests added ----------- _cobsEncodedMaxLength: Tests at exact multiples of 254 (254, 508) that fail with ceil() but pass with floor()+1. Also tests at 253 and 255 to confirm non-boundary values are unaffected. _cobsGuaranteedFit: Tests at 255k+1 buffer sizes (256, 511, 766) that fail with the old formula. Also tests at 257 (non-boundary) and 262,144 (the customer's actual buffer size) to anchor the expected behavior. Round-trip consistency: For a range of buffer sizes -- including every 255k and 255k+1 value from 0 to 2,550, plus large values up to 262,144 -- the tests verify that _cobsEncodedMaxLength(_cobsGuaranteedFit(B)) never exceeds B. This ensures the two functions agree with each other and that the guaranteed-fit promise is upheld. --- n_cobs.c | 80 ++++++++++--------- test/src/_cobsEncodedMaxLength_test.cpp | 77 ++++++++++++++++++ test/src/_cobsGuaranteedFit_test.cpp | 100 ++++++++++++++++++++++++ 3 files changed, 222 insertions(+), 35 deletions(-) diff --git a/n_cobs.c b/n_cobs.c index e7558e77..adaa9577 100644 --- a/n_cobs.c +++ b/n_cobs.c @@ -244,20 +244,26 @@ uint32_t _cobsEncodedLength(const uint8_t *ptr, uint32_t length) @param length Length of the data to encode - @return The max length required to encode the data + @return The max length required to encode the data (including EOP byte) + + @note Worst case is input with no zero bytes (no EOP markers to replace). + The COBS encoder always emits 1 initial code byte, then 1 additional + code byte for every 254 data bytes processed: + + codeBytes = floor(length / 254) + 1 + + IMPORTANT: This is NOT ceil(length / 254). They differ at exact + multiples of 254. Example: encoding 254 non-zero bytes produces + code byte 0xFF, 254 data bytes, then a final code byte 0x01. + That's 2 code bytes, but ceil(254/254) = 1 (wrong). - @note Since the contents of the buffer are unknown, then we must assume - that the entire buffer has no end-of-packet markers. This would - require the injection of overhead bytes (as opposed to the - replacement of end-of-packet markers with overhead bytes) at - intervals of 255, thus producing the worst case scenario. @note An additional byte is added for the EOP (end-of-packet) marker. */ /**************************************************************************/ uint32_t _cobsEncodedMaxLength(uint32_t length) { - const uint32_t overheadBytes = (length == 0) + ((length != 0) * ((length / COBS_MAX_PACKET_SIZE) + ((length % COBS_MAX_PACKET_SIZE) > 0))); - return (length + overheadBytes + COBS_EOP_OVERHEAD); + const uint32_t codeBytes = (length / COBS_MAX_PACKET_SIZE) + 1; + return (length + codeBytes + COBS_EOP_OVERHEAD); } //**************************************************************************/ @@ -267,36 +273,40 @@ uint32_t _cobsEncodedMaxLength(uint32_t length) @param bufLen Length of the buffer in bytes - @return the length of unencoded data + @return the length of unencoded data that is guaranteed to fit when + COBS-encoded into bufLen bytes (including EOP) + + @note The COBS encoder always emits 1 initial code byte, then 1 additional + code byte for every 254 data bytes. Therefore: + + codeBytes(u) = floor(u / 254) + 1 + + IMPORTANT: This is NOT ceil(u / 254). They differ at exact multiples + of 254. Example: encoding 254 non-zero bytes produces code 0xFF, + 254 data bytes, then final code 0x01 — that's 2 code bytes, but + ceil(254/254) = 1 (wrong). - @note An additional byte for the EOP (end-of-packet) marker is assumed. + Buffer requirement: bufLen >= u + floor(u/254) + 1 + 1(EOP) + = u + floor(u/254) + 2 + + Inversion: find max u where u + floor(u/254) <= t, with t = bufLen-2. + Substituting u = 254q + r (0 <= r <= 253): 255q + r <= t. + + Closed form: u = t - floor((t + 1) / 255) + + Proof: Let t+1 = 255k + j (0 <= j <= 254), so u = 254k + j - 1. + j >= 1: floor(u/254)=k, u+floor(u/254) = 255k+j-1 = t. Exact fit. + j = 0: u=254(k-1)+253, floor(u/254)=k-1, sum = t-1. One byte slack. + In both cases u+1 would exceed t, confirming u is the maximum. + + @see _cobsEncodedMaxLength() */ /**************************************************************************/ uint32_t _cobsGuaranteedFit(uint32_t bufLen) { - // encodedLen = unencodedLen + codeBytesLen - // e = u + c (e = encoded (sorry Euler), and c = code bytes (sorry Einstein)) - // u = e - c - // c = ⌈u / 254⌉ (the ceiling of u divided by 254) - // - // Rearranging the ceiling equation: - // (c - 1) < u / 254 <= c - // 254(c - 1) < u <= 254c - // - // Substitute u from first equation: - // 254(c - 1) < e - c <= 254c - // 254c - 254 < e - c <= 254c - // 255c < e + 254 AND e <= 255c - // - // Thus: - // e <= 255c < e + 254 - // e / 255 <= c < (e + 254) / 255 - // - // Knowing that c is an integer, we can express c as: - // c = ⌊(e + 254) / 255⌋ (the floor of (e + 254) divided by 255) - // - // Substitute c back into the original equation for u: - // u = e - ⌊(e + 254) / 255⌋ - const uint32_t encodedLen = (bufLen == 0) + ((bufLen != 0) * (bufLen - COBS_EOP_OVERHEAD)); - return (encodedLen - ((encodedLen + COBS_MAX_PACKET_SIZE) / 255)); + if (bufLen <= 2) { + return 0; + } + const uint32_t t = bufLen - 2; + return (t - ((t + 1) / 255)); } diff --git a/test/src/_cobsEncodedMaxLength_test.cpp b/test/src/_cobsEncodedMaxLength_test.cpp index 47244c9e..a4c3a66a 100644 --- a/test/src/_cobsEncodedMaxLength_test.cpp +++ b/test/src/_cobsEncodedMaxLength_test.cpp @@ -74,6 +74,83 @@ SCENARIO("_cobsEncodedMaxLength") } } } + + // The following tests verify the fix for the ceil(u/254) vs floor(u/254)+1 + // bug. At exact multiples of 254, the encoder emits an extra code byte + // that ceil() misses. Example: 254 non-zero bytes encode as [0xFF, data + // x254, 0x01] — that's 2 code bytes, not ceil(254/254)=1. + + GIVEN("A buffer length of exactly 253 bytes (just under one full COBS block)") { + const uint32_t bufLen = 253; + + WHEN("The max encoded length is calculated") { + const uint32_t encodedLen = _cobsEncodedMaxLength(bufLen); + + THEN("The encoded length is 253 + 1 code byte + 1 EOP = 255") { + CHECK(encodedLen == 255); + } + } + } + + GIVEN("A buffer length of exactly 254 bytes (one full COBS block)") { + // BUG FIX: Old code used ceil(254/254)=1 code byte, but the encoder + // actually emits 2: one 0xFF at the start and one 0x01 at the end. + const uint32_t bufLen = 254; + + WHEN("The max encoded length is calculated") { + const uint32_t encodedLen = _cobsEncodedMaxLength(bufLen); + + THEN("The encoded length is 254 + 2 code bytes + 1 EOP = 257") { + CHECK(encodedLen == 257); + } + } + } + + GIVEN("A buffer length of exactly 508 bytes (two full COBS blocks)") { + // BUG FIX: Old code used ceil(508/254)=2 code bytes, but the encoder + // emits 3: one per 254-byte block plus one final. + const uint32_t bufLen = 508; + + WHEN("The max encoded length is calculated") { + const uint32_t encodedLen = _cobsEncodedMaxLength(bufLen); + + THEN("The encoded length is 508 + 3 code bytes + 1 EOP = 512") { + CHECK(encodedLen == 512); + } + } + } + + GIVEN("A buffer length of 255 bytes (one byte past a full COBS block)") { + const uint32_t bufLen = 255; + + WHEN("The max encoded length is calculated") { + const uint32_t encodedLen = _cobsEncodedMaxLength(bufLen); + + THEN("The encoded length is 255 + 2 code bytes + 1 EOP = 258") { + CHECK(encodedLen == 258); + } + } + } +} + +SCENARIO("_cobsEncodedMaxLength round-trip with _cobsGuaranteedFit") +{ + GIVEN("Various buffer sizes") { + // For any buffer size B, encoding cobsGuaranteedFit(B) bytes must + // produce at most B bytes (including the EOP byte). + uint32_t testSizes[] = {3, 8, 255, 256, 257, 510, 511, 512, 65536, 65796, 262144}; + + WHEN("_cobsGuaranteedFit is computed and then _cobsEncodedMaxLength is applied") { + THEN("The encoded max length never exceeds the original buffer") { + for (size_t i = 0; i < sizeof(testSizes)/sizeof(testSizes[0]); i++) { + uint32_t bufLen = testSizes[i]; + uint32_t maxUnencoded = _cobsGuaranteedFit(bufLen); + uint32_t encodedLen = _cobsEncodedMaxLength(maxUnencoded); + CHECK(encodedLen <= bufLen); + } + } + } + } } } diff --git a/test/src/_cobsGuaranteedFit_test.cpp b/test/src/_cobsGuaranteedFit_test.cpp index 4e75e4dc..b2a06d2e 100644 --- a/test/src/_cobsGuaranteedFit_test.cpp +++ b/test/src/_cobsGuaranteedFit_test.cpp @@ -100,6 +100,106 @@ SCENARIO("_cobsGuaranteedFit") } } } + + // The following tests verify the fix for the ceil(u/254) vs floor(u/254)+1 + // bug. At bufLen = 255k+1, the old formula overestimated by 1, promising a + // fit that would actually overflow the buffer after COBS encoding. + + GIVEN("A buffer length of 256 bytes (255*1 + 1, boundary case)") { + // BUG FIX: Old code returned 254. But encoding 254 non-zero bytes + // produces 254 + 2 code bytes = 256, which with +1 EOP = 257 > 256. + // Correct answer: 253 (encodes to 253 + 1 code + 1 EOP = 255 ≤ 256). + const uint32_t bufLen = 256; + + WHEN("The max unencoded length is calculated") { + const uint32_t unencodedLen = _cobsGuaranteedFit(bufLen); + + THEN("The unencoded length is 253, not 254") { + CHECK(unencodedLen == 253); + } + } + } + + GIVEN("A buffer length of 511 bytes (255*2 + 1, boundary case)") { + // BUG FIX: Old code returned 508. But encoding 508 non-zero bytes + // produces 508 + 3 code bytes = 511, plus 1 EOP = 512 > 511. + // Correct answer: 507. + const uint32_t bufLen = 511; + + WHEN("The max unencoded length is calculated") { + const uint32_t unencodedLen = _cobsGuaranteedFit(bufLen); + + THEN("The unencoded length is 507, not 508") { + CHECK(unencodedLen == 507); + } + } + } + + GIVEN("A buffer length of 766 bytes (255*3 + 1, boundary case)") { + const uint32_t bufLen = 766; + + WHEN("The max unencoded length is calculated") { + const uint32_t unencodedLen = _cobsGuaranteedFit(bufLen); + + THEN("The unencoded length is 761") { + CHECK(unencodedLen == 761); + } + } + } + + GIVEN("A buffer length of 257 bytes (non-boundary, one past 256)") { + const uint32_t bufLen = 257; + + WHEN("The max unencoded length is calculated") { + const uint32_t unencodedLen = _cobsGuaranteedFit(bufLen); + + THEN("The unencoded length is 254") { + CHECK(unencodedLen == 254); + } + } + } + + GIVEN("A buffer length of 262144 bytes (customer-reported capacity)") { + const uint32_t bufLen = 262144; + + WHEN("The max unencoded length is calculated") { + const uint32_t unencodedLen = _cobsGuaranteedFit(bufLen); + + THEN("The unencoded length is 261114") { + CHECK(unencodedLen == 261114); + } + } + } +} + +SCENARIO("_cobsGuaranteedFit round-trip with _cobsEncodedMaxLength") +{ + GIVEN("Buffer sizes at every 255k+1 boundary from 1 to 2551") { + WHEN("_cobsGuaranteedFit is computed and then _cobsEncodedMaxLength is applied") { + THEN("The encoded max length never exceeds the original buffer") { + for (uint32_t k = 0; k <= 10; k++) { + uint32_t bufLen = 255 * k + 1; + if (bufLen <= 2) continue; + uint32_t maxUnencoded = _cobsGuaranteedFit(bufLen); + uint32_t encodedLen = _cobsEncodedMaxLength(maxUnencoded); + CHECK(encodedLen <= bufLen); + } + } + } + } + + GIVEN("Buffer sizes at every 255k boundary from 255 to 2550") { + WHEN("The result is checked against _cobsEncodedMaxLength") { + THEN("The encoded max length never exceeds the original buffer") { + for (uint32_t k = 1; k <= 10; k++) { + uint32_t bufLen = 255 * k; + uint32_t maxUnencoded = _cobsGuaranteedFit(bufLen); + uint32_t encodedLen = _cobsEncodedMaxLength(maxUnencoded); + CHECK(encodedLen <= bufLen); + } + } + } + } } }