Skip to content

fix: resolve out-of-bounds reads, uninitialized decfrac scaling, string termination, and lock handling - #43

Open
xPuffyPuffx wants to merge 1 commit into
ThingSet:mainfrom
xPuffyPuffx:fix-bugs-public
Open

xPuffyPuffx wants to merge 1 commit into
ThingSet:mainfrom
xPuffyPuffx:fix-bugs-public

Conversation

@xPuffyPuffx

Copy link
Copy Markdown

Summary

While running unit conformance tests and fuzzing passes with Clang AddressSanitizer (CONFIG_ASAN=y) and UndefinedBehaviorSanitizer (CONFIG_UBSAN=y), we identified several edge-case handling bugs across CBOR/JSON deserialization and helper functions:

  1. Bounds-checked path traversal for CBOR strings (thingset.c:626):
    CBOR text strings decoded via zcbor_tstr_decode are length-delimited (path_len) and not null-terminated. In thingset_get_object_by_path(), calling end = strchr(start, '/') when evaluating the leaf component (no trailing '/') scanned past path + path_len into adjacent memory.
    Fix: Replaced strchr(start, '/') with memchr(start, '/', path + path_len - start).

  2. Check zcbor decode status in DECFRAC deserialization (thingset_bin.c:606):
    If a DECFRAC (CBOR Tag 4) payload list is malformed (e.g. empty list 0xC4 0x80), zcbor_list_start_decode() fails, setting success = false. Because exponent_received and mantissa_tmp were decoded via short-circuiting success && ..., they remained uninitialized on the stack. The scaling for loops and *mantissa = mantissa_tmp executed unconditionally using uninitialized stack values.
    Fix: Initialized temporary variables to zero and wrapped the scaling loop and assignment inside if (success).

  3. Null-terminate unescaped JSON strings (thingset_txt.c:612):
    Under CONFIG_THINGSET_JSON_STRING_ESCAPING=y, character unescaping (\", \/, etc.) wrote into data.str[data_pos++] but omitted writing a trailing \0. If an unescaped string was shorter than the previous contents of the buffer, residual characters from previous writes remained visible.
    Fix: Added data.str[data_pos] = '\0'; immediately after loop completion.

  4. Release context semaphore on unsupported export format (thingset.c:197):
    thingset_export_subsets() acquires k_sem_take(&ts->lock, ...). If an unsupported format enum is passed, the default: branch returned -THINGSET_ERR_NOT_IMPLEMENTED without calling k_sem_give(&ts->lock), causing subsequent context operations to block indefinitely on lock timeout.
    Fix: Added k_sem_give(&ts->lock); before returning from default:.

  5. Fix buffer bounds calculations in thingset_get_type_name and get_function_arg_types (thingset.c:797, 811):

    • In thingset_get_type_name(), sizeof(elementType) > size evaluated sizeof(char *) (4 or 8 bytes) instead of strlen(elementType) + 3 > size.
    • In get_function_arg_types(), if (total_len > size) compared accumulated total bytes against the remaining buffer capacity (size decremented by len).

Verification

All existing and boundary unit tests pass cleanly under native_sim/native/64 with CONFIG_ASAN=y and CONFIG_UBSAN=y: Running TESTSUITE thingset_audit_core ... PASS Running TESTSUITE thingset_bin ... PASS Running TESTSUITE thingset_txt ... PASS PROJECT EXECUTION SUCCESSFUL

…uffer handling

- Replace strchr with memchr in thingset_get_object_by_path to prevent
  reading past length-delimited CBOR strings.
- Initialize decfrac decode variables and check zcbor status before
  scaling mantissa to prevent uninitialized loop execution.
- Add null-termination to unescaped JSON strings in txt_deserialize_value.
- Release ts->lock on unsupported format in thingset_export_subsets.
- Fix sizeof(pointer) and accumulated size checks in type name helpers.

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