Skip to content

Comments

fix: correct memcmp misplaced parentheses (size argument in _crcError CRC field detection)#224

Open
m-mcgowan wants to merge 2 commits intoblues:masterfrom
m-mcgowan:fix/crc-memcmp-bug
Open

fix: correct memcmp misplaced parentheses (size argument in _crcError CRC field detection)#224
m-mcgowan wants to merge 2 commits intoblues:masterfrom
m-mcgowan:fix/crc-memcmp-bug

Conversation

@m-mcgowan
Copy link
Contributor

Summary

  • Fix misplaced parenthesis in _crcError where the memcmp return value is not checked correctly — != 0 was intended to test whether memcmp found a match, but instead it's parsed as part of the size argument, reducing a 7-byte comparison to 1 byte
  • Add regression tests using a realistic hub.status response that triggers the false detection

The Bug

_crcError detects CRC fields by checking 22 bytes before the closing } for the pattern "crc":". The memcmp call was:

memcmp(..., CRC_FIELD_NAME_TEST, ((sizeof(CRC_FIELD_NAME_TEST) - 1)) != 0))

The != 0 was intended to test the memcmp return value, but the closing parentheses are misplaced — it's parsed as part of the third argument: (7 != 0) = 1. So memcmp only compared 1 byte — the opening " quote — which matches ANY JSON field boundary at that position.

When does this trigger?

Only when talking to a Notecard that does not support CRC. note-c always adds CRC to outgoing requests speculatively. When the Notecard supports CRC, it responds with a real ,"crc":"..." field at the expected offset, so the 1-byte compare accidentally matches the correct field and everything works.

When the Notecard does not support CRC, responses lack a CRC field. If the response happens to place a " character at the CRC detection offset (22 bytes before the closing }), the 1-byte compare falsely matches it.

For example, {"connected":false,"status":"connecting"} (41 bytes) — a real hub.status response during connection — has " at position 19 (= 41 - 22), the opening quote of "status". This is a realistic trigger because hub.status only returns connected (boolean) and status (string) with no other fields. This triggers false CRC detection, which:

  1. Permanently sets notecardFirmwareSupportsCrc = true
  2. Every subsequent response without a real CRC field is then flagged as a CRC error
  3. Triggers retries until failure, effectively bricking communication

Fix

// Before (buggy — != 0 applied to size arg, evaluates to 1):
memcmp(..., CRC_FIELD_NAME_TEST, ((sizeof(CRC_FIELD_NAME_TEST) - 1)) != 0))
// After (fixed — != 0 applied to memcmp return value):
memcmp(..., CRC_FIELD_NAME_TEST, sizeof(CRC_FIELD_NAME_TEST) - 1) != 0)

Test plan

  • Existing _crcError_test tests pass (CRC detection, validation, edge cases)
  • New regression test: hub.status response does NOT trigger false CRC detection when firmware does not support CRC
  • New regression test: notecardFirmwareSupportsCrc is not falsely set to true
  • When firmware supports CRC, missing CRC is correctly flagged as error
  • CI passes

The third argument to memcmp was ((sizeof(CRC_FIELD_NAME_TEST) - 1)) != 0)
which evaluates != 0 as part of the size expression, yielding boolean 1
instead of the intended length 7. This caused CRC field detection to
compare only 1 byte (a quote character) instead of the full pattern.

This is triggered on Notecard firmware that does not support CRC. A
hub.status response like {"connected":false,"status":"connecting"}
has a quote at the CRC detection offset (position 19 = 41 - 22). The
1-byte compare falsely matches it as a CRC field, permanently setting
notecardFirmwareSupportsCrc=true and causing all subsequent responses
to be flagged as CRC errors.

When the Notecard does support CRC, the real CRC field sits at the
expected offset, so the 1-byte compare accidentally matches correctly.

Add regression tests with a realistic hub.status response that triggers
the bug to verify the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant