Skip to content

Conversation

sirknightj
Copy link
Contributor

Issue #, if available:

What was changed?

  1. Removed 'ice server config' parsing logic from the signaling messages.
  2. Added new parseSignalingMessage() function to separate message parsing logic
  3. Added comprehensive test cases for signaling message parsing
  4. Changed tokenCount to signed int32 from unsigned
  5. Decreased MAX_STATUS_CODE_STRING_LEN from 256 to 8 bytes

Why was it changed?

  1. We use GetIceServerConfig to get the TURN details, they are not received from Signaling.
  2. To improve code modularity by separating message parsing logic from message handling
  3. To add robust validation of message fields to prevent buffer overflows and undefined behavior
  4. To make sure all errors are gracefully handled (instead of crashing the application)
  5. To save a tiny bit of memory.

How was it changed?

  1. Those paths were removed and the related unit tests checking that were also removed.
  2. The new method parseSignalingMessage():
    • Uses SNPRINTF instead of STRNCPY with manual null-termination.
    • Null-terminates where the string ends instead of the last spot in the buffer (eventually setting up for utilizing lazy memory allocations if the full buffer isn't needed)
  3. Added test cases covering:
    • Successful parsing of offer messages
    • Successful parsing of status response messages
    • Invalid message formats
    • Field length validation
    • Empty messages
    • Extra fields handling
  4. The jsmn_parse can return negative error codes, so it was turned to signed int32. If a negative error code was returned, it would previously be interpreted as a very large int.
  5. Since HTTP codes like '400' are returned, it doesn't need to be 256 bytes long. The buffer only needs to be 4 bytes long. Added a bit of extra padding just in case.

What testing was done for the changes?

  • Run the added the new unit tests for the various invalid JSON scenarios such as empty string, missing fields, etc.
  • Manually checked the P2P connection with the JS page
  • CI

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@stefankiesz stefankiesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. An issue with the existing design of the tests is that we rely on STATUS_INVALID_API_CALL_RETURN_JSON error code return for many reasons for which the return JSON may be invalid, so there is some ambiguity there that could hide improper logic in the tested code. Can be a future improvement.

@sirknightj sirknightj requested a review from stefankiesz June 17, 2025 03:55
@sirknightj sirknightj merged commit 16c40d4 into develop Jun 17, 2025
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants