Skip to content

fix: fractional seconds bounds check and TriggerMessage status handling - #461

Open
xPuffyPuffx wants to merge 1 commit into
matth-x:mainfrom
xPuffyPuffx:fix-time-and-trigger
Open

xPuffyPuffx wants to merge 1 commit into
matth-x:mainfrom
xPuffyPuffx:fix-time-and-trigger

Conversation

@xPuffyPuffx

Copy link
Copy Markdown

Summary

This PR addresses two issues identified during conformance and sanitizer testing with AddressSanitizer and UndefinedBehaviorSanitizer:

  1. Fractional seconds bounds check and validation in Timestamp::setTime (Time.cpp:82):
    When parsing fractional seconds (jsonDateString[19] == '.'), the validator checked:

    if (isdigit(jsonDateString[20]) ||
        isdigit(jsonDateString[21]) ||
        isdigit(jsonDateString[22]))

    Using logical OR (||) instead of logical AND (&&) caused two problems:

    • For truncated strings like "2024-01-01T12:00:00." (length 20) or "2024-01-01T12:00:00.1" (length 21), indices 21 and 22 read 1–2 bytes past the null terminator (global/stack buffer overread caught by ASan).
    • If non-digit characters were present (e.g. "2024-01-01T12:00:00.1abZ"), (c - '0') evaluated arbitrary ASCII characters as digits, generating corrupted millisecond values.
      Fix: Replaced || with &&. Because && short-circuits in C++ and isdigit('\0') is false, truncated strings stop immediately without reading past the buffer boundary.
  2. Return NotImplemented for unregistered operations in TriggerMessage (TriggerMessage.cpp:70):
    OperationRegistry::deserializeOperation() returns a Request wrapping new NotImplemented() rather than nullptr when an operation is unregistered.
    Because if (msg) was always true, TriggerMessage erroneously set statusMessage = "Accepted" and queued the NotImplemented object into preBootSendQueue, which later caused Operation::createReq() to emit Unsupported operation: createReq() is not implemented and transmit a bogus empty CustomOperation Call frame.
    Fix: Checked if (msg && !msg->getOperation()->getErrorCode()) before accepting and queuing the operation.

Verification

Tested with host Clang, ASan, and UBSan against boundary timestamps ("2024-01-01T12:00:00.", "2024-01-01T12:00:00.1", "2024-01-01T12:00:00.123Z") and unregistered TriggerMessage requests. All unit tests pass with 0 sanitizer warnings.

- Ensure all fractional second characters in Timestamp::setTime are
  digits using conjunction &&, preventing out-of-bounds reads past string
  terminator and math corruption.
- Check if deserialized operation carries an error code in TriggerMessage,
  returning NotImplemented instead of queuing an unsupported operation.

This branch has not been deployed

No deployments
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