Skip to content

Enhance the ice config parser #2127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

sirknightj
Copy link
Contributor

Issue #, if available:

  • N/A

What was changed?

  • Refactored ICE server configuration parsing into a dedicated function parseIceConfigResponse
  • Fixed potential buffer overflow in string handling
  • Added proper buffer length macros to clearly distinguish between string length and buffer size
  • Improved error handling for malformed JSON responses
  • Fixed incorrect array indexing in password field null termination

Why was it changed?

  • Initially investigated to fix a bug where password field wasn't being properly null-terminated:
// Before: Incorrectly using userName instead of password
pSignalingClient->iceConfigs[configCount - 1].userName[MAX_ICE_CONFIG_CREDENTIAL_LEN] = '\0';

Note

This fixes a potential non-null terminated password string. This is not currently an issue since the buffer is 257 long (to match 256 limit). From calling getIceServer config a handful of times, the passwords returned to me were all under 50 characters long. The buffer is memset to 0 beforehand, so the string is always currently null-terminated.

  • While fixing this, identified several opportunities for improvement:
    • String handling safety: Previous implementation used STRNCPY which required manual null termination
    • Buffer size clarity: Added explicit BUFFER_LEN macros to distinguish between content length and total buffer size
    • JSON parsing robustness: Made tokenCount signed to properly handle jsmn_parse error codes
    • Code maintainability: Extracted parsing logic to separate function for better testing and readability

How was it changed?

  • Introduced new BUFFER_LEN macros (e.g., MAX_ICE_CONFIG_URI_BUFFER_LEN) to explicitly include null terminator
  • Replaced STRNCPY with SNPRINTF for safer string handling
  • Added comprehensive error handling for malformed JSON
  • Made tokenCount signed (INT32) instead of UINT32 to properly handle JSON parser error codes
  • Created dedicated unit tests to verify parsing behavior

What testing was done for the changes?

  • Added comprehensive unit test suite (IceConfigParsingTest) covering:
    • Successful parsing scenarios (happy path)
    • Invalid argument handling
    • Buffer overflow prevention
    • Malformed JSON handling
    • Extra field handling
    • Maximum URI/config count and length handling

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

@sirknightj sirknightj added bug Something isn't working Tests v1.13.0 labels May 7, 2025
@sirknightj sirknightj force-pushed the ice-config-parser-and-tests branch from 2f9b7e5 to c66c009 Compare May 7, 2025 06:36
@sirknightj sirknightj changed the base branch from release-v1.12.1 to develop May 7, 2025 22:40
@sirknightj sirknightj force-pushed the ice-config-parser-and-tests branch from c66c009 to 6158ab6 Compare May 7, 2025 22:44
@sirknightj
Copy link
Contributor Author

sirknightj commented May 15, 2025

Adjusted the logic based on suggestion in: #2139

I did also check:

  • Master (C) to JS viewer with ForceTURN and setting the max ice configs to 1 and observed a successful relay-relay connection and playback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Tests v1.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant