Enhance the ice config parser (#2127)

sirknightj · vikramdattu · web-flow · commit 4722375a6aea · 2025-05-28T10:17:08.000-07:00
* Move the ICE config parser to its own method and add tests

* Add cases for invalid JSONs

* Exit parsing ice servers early if the array if we requested less configs

Co-authored-by: Vikram Dattu &lt;vikram.dattu@espressif.com&gt;

---------

Co-authored-by: Vikram Dattu &lt;vikram.dattu@espressif.com&gt;
diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h
@@ -449,12 +449,27 @@ extern "C" {
  */
 #define MAX_ICE_CONFIG_USER_NAME_LEN 256
 
+/**
+ * Buffer length for ICE configuration user name, including null terminator.
+ *
+ * \sa MAX_ICE_CONFIG_USER_NAME_LEN
+ */
+#define MAX_ICE_CONFIG_USER_NAME_BUFFER_LEN (MAX_ICE_CONFIG_USER_NAME_LEN + 1)
+
 /**
  * Maximum allowed ICE configuration password length
- * https://docs.aws.amazon.com/kinesisvideostreams/latest/dg/API_AWSAcuitySignalingService_IceServer.html#KinesisVideo-Type-AWSAcuitySignalingService_IceServer-Password
+ *
+ * \sa https://docs.aws.amazon.com/kinesisvideostreams/latest/dg/API_signaling_IceServer.html#KinesisVideo-Type-signaling_IceServer-Password
  */
 #define MAX_ICE_CONFIG_CREDENTIAL_LEN 256
 
+/**
+ * Buffer length for ICE configuration password, including null terminator.
+ *
+ * \sa MAX_ICE_CONFIG_USER_NAME_LEN
+ */
+#define MAX_ICE_CONFIG_CREDENTIAL_BUFFER_LEN (MAX_ICE_CONFIG_CREDENTIAL_LEN + 1)
+
 /**
  * Maximum allowed signaling URI length
  */
@@ -1139,9 +1154,9 @@ typedef struct {
  * https://www.w3.org/TR/webrtc/#rtciceserver-dictionary
  */
 typedef struct {
-    CHAR urls[MAX_ICE_CONFIG_URI_LEN + 1];              //!< URL of STUN/TURN Server
-    CHAR username[MAX_ICE_CONFIG_USER_NAME_LEN + 1];    //!< Username to be used with TURN server
-    CHAR credential[MAX_ICE_CONFIG_CREDENTIAL_LEN + 1]; //!< Password to be used with TURN server
+    CHAR urls[MAX_ICE_CONFIG_URI_LEN + 1];                 //!< URL of STUN/TURN Server
+    CHAR username[MAX_ICE_CONFIG_USER_NAME_BUFFER_LEN];    //!< Username to be used with TURN server
+    CHAR credential[MAX_ICE_CONFIG_CREDENTIAL_BUFFER_LEN]; //!< Password to be used with TURN server
 } RtcIceServer, *PRtcIceServer;
 
 /**
@@ -1390,12 +1405,12 @@ typedef struct {
  * NOTE:TTL is given in default which is 100ns duration
  */
 typedef struct {
-    UINT32 version;                                                  //!< Version of the struct
-    UINT64 ttl;                                                      //!< TTL of the configuration is 100ns
-    UINT32 uriCount;                                                 //!<  Number of Ice URI objects
-    CHAR uris[MAX_ICE_CONFIG_URI_COUNT][MAX_ICE_CONFIG_URI_LEN + 1]; //!< List of Ice server URIs
-    CHAR userName[MAX_ICE_CONFIG_USER_NAME_LEN + 1];                 //!< Username for the server
-    CHAR password[MAX_ICE_CONFIG_CREDENTIAL_LEN + 1];                //!< Password for the server
+    UINT32 version;                                                     //!< Version of the struct
+    UINT64 ttl;                                                         //!< TTL of the configuration is 100ns
+    UINT32 uriCount;                                                    //!<  Number of Ice URI objects
+    CHAR uris[MAX_ICE_CONFIG_URI_COUNT][MAX_ICE_CONFIG_URI_BUFFER_LEN]; //!< List of Ice server URIs
+    CHAR userName[MAX_ICE_CONFIG_USER_NAME_BUFFER_LEN];                 //!< Username for the server
+    CHAR password[MAX_ICE_CONFIG_CREDENTIAL_BUFFER_LEN];                //!< Password for the server
 } IceConfigInfo, *PIceConfigInfo;
 
 typedef struct {
diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h
@@ -29,6 +29,13 @@ extern "C" {
  */
 #define MAX_ICE_CONFIG_URI_LEN 127
 
+/**
+ * Maximum allowed ICE URI buffer length, including the null terminator.
+ *
+ * \sa MAX_ICE_CONFIG_URI_LEN
+ */
+#define MAX_ICE_CONFIG_URI_BUFFER_LEN (MAX_ICE_CONFIG_URI_LEN + 1)
+
 /**
  * Maximum allowed relay protocol length
  */
@@ -244,14 +251,14 @@ typedef struct {
  * Reference: https://www.w3.org/TR/webrtc-stats/#ice-server-dict*
  */
 typedef struct {
-    CHAR url[MAX_ICE_CONFIG_URI_LEN + 1];   //!< STUN/TURN server URL
-    CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP
-    UINT32 iceServerIndex;                  //!< Ice server index to get stats from. Not available in spec! Needs to be
-                                            //!< populated by the application to get specific server stats
-    INT32 port;                             //!< Port number used by client
-    UINT64 totalRequestsSent;               //!< Total amount of requests that have been sent to the server
-    UINT64 totalResponsesReceived;          //!< Total number of responses received from the server
-    UINT64 totalRoundTripTime;              //!< Sum of RTTs of all the requests for which response has been received
+    CHAR url[MAX_ICE_CONFIG_URI_BUFFER_LEN]; //!< STUN/TURN server URL
+    CHAR protocol[MAX_PROTOCOL_LENGTH + 1];  //!< Valid values: UDP, TCP
+    UINT32 iceServerIndex;                   //!< Ice server index to get stats from. Not available in spec! Needs to be
+                                             //!< populated by the application to get specific server stats
+    INT32 port;                              //!< Port number used by client
+    UINT64 totalRequestsSent;                //!< Total amount of requests that have been sent to the server
+    UINT64 totalResponsesReceived;           //!< Total number of responses received from the server
+    UINT64 totalRoundTripTime;               //!< Sum of RTTs of all the requests for which response has been received
 } RtcIceServerStats, *PRtcIceServerStats;
 
 /**
diff --git a/src/source/Ice/IceUtils.h b/src/source/Ice/IceUtils.h
@@ -54,10 +54,10 @@ typedef struct {
     // isTurn ? TURN server : STUN server
     BOOL isTurn;
     BOOL isSecure;
-    CHAR url[MAX_ICE_CONFIG_URI_LEN + 1];
+    CHAR url[MAX_ICE_CONFIG_URI_BUFFER_LEN];
     KvsIpAddress ipAddress;
-    CHAR username[MAX_ICE_CONFIG_USER_NAME_LEN + 1];
-    CHAR credential[MAX_ICE_CONFIG_CREDENTIAL_LEN + 1];
+    CHAR username[MAX_ICE_CONFIG_USER_NAME_BUFFER_LEN];
+    CHAR credential[MAX_ICE_CONFIG_CREDENTIAL_BUFFER_LEN];
     KVS_SOCKET_PROTOCOL transport;
     IceServerSetIpFunc setIpFn;
 } IceServer, *PIceServer;
diff --git a/src/source/Signaling/LwsApiCalls.c b/src/source/Signaling/LwsApiCalls.c
@@ -1188,14 +1188,47 @@ STATUS getIceConfigLws(PSignalingClient pSignalingClient, UINT64 time)
     CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL,
         STATUS_SIGNALING_LWS_CALL_FAILED);
 
-    // Parse the response
+    CHK_STATUS(
+        parseIceConfigResponse(pResponseStr, resultLen, MAX_ICE_CONFIG_COUNT, pSignalingClient->iceConfigs, &pSignalingClient->iceConfigCount));
+
+    // Perform some validation on the ice configuration
+    CHK_STATUS(validateIceConfiguration(pSignalingClient));
+
+CleanUp:
+
+    if (STATUS_FAILED(retStatus)) {
+        DLOGE("Call Failed with Status:  0x%08x", retStatus);
+    }
+
+    freeLwsCallInfo(&pLwsCallInfo);
+
+    LEAVES();
+    return retStatus;
+}
+
+STATUS parseIceConfigResponse(PCHAR pResponseStr, UINT32 responseLen, UINT8 maxIceConfigs, PIceConfigInfo pIceConfigs, PUINT32 pIceConfigCount)
+{
+    ENTERS();
+    STATUS retStatus = STATUS_SUCCESS;
+    jsmn_parser parser;
+    jsmntok_t tokens[MAX_JSON_TOKEN_COUNT];
+    jsmntok_t* pToken;
+    UINT32 i, configCount = 0, strLen; // Note: strLen excludes the NULL terminator
+    INT32 j, tokenCount;
+    UINT64 ttl;
+    BOOL jsonInIceServerList = FALSE;
+
+    CHK(pIceConfigs != NULL && pIceConfigCount != NULL && pResponseStr != NULL, STATUS_NULL_ARG);
+    CHK(maxIceConfigs > 0, STATUS_INVALID_ARG);
+    CHK(!IS_EMPTY_STRING(pResponseStr), STATUS_INVALID_API_CALL_RETURN_JSON);
+
     jsmn_init(&parser);
-    tokenCount = jsmn_parse(&parser, pResponseStr, resultLen, tokens, SIZEOF(tokens) / SIZEOF(jsmntok_t));
+    tokenCount = jsmn_parse(&parser, pResponseStr, responseLen, tokens, SIZEOF(tokens) / SIZEOF(jsmntok_t));
     CHK(tokenCount > 1, STATUS_INVALID_API_CALL_RETURN_JSON);
     CHK(tokens[0].type == JSMN_OBJECT, STATUS_INVALID_API_CALL_RETURN_JSON);
 
-    MEMSET(&pSignalingClient->iceConfigs, 0x00, MAX_ICE_CONFIG_COUNT * SIZEOF(IceConfigInfo));
-    pSignalingClient->iceConfigCount = 0;
+    MEMSET(pIceConfigs, 0x00, maxIceConfigs * SIZEOF(IceConfigInfo));
+    *pIceConfigCount = 0;
 
     // Loop through the tokens and extract the ice configuration
     for (i = 0; i < tokenCount; i++) {
@@ -1204,58 +1237,64 @@ STATUS getIceConfigLws(PSignalingClient pSignalingClient, UINT64 time)
                 jsonInIceServerList = TRUE;
 
                 CHK(tokens[i + 1].type == JSMN_ARRAY, STATUS_INVALID_API_CALL_RETURN_JSON);
-                CHK(tokens[i + 1].size <= MAX_ICE_CONFIG_COUNT, STATUS_SIGNALING_MAX_ICE_CONFIG_COUNT);
+                if (tokens[i + 1].size > maxIceConfigs) {
+                    DLOGW("Received more ice configs (%d) than supported (%d). Will ignore the rest.", tokens[i + 1].size, maxIceConfigs);
+                }
             }
         } else {
             pToken = &tokens[i];
-            if (pToken->type == JSMN_OBJECT) {
+            if (pToken->type == JSMN_UNDEFINED) {
+                DLOGW("Encountered unexpected item in the JSON! %*.s", pResponseStr, responseLen);
+                // Skip this token and continue parsing
+                // Don't return error, just move to next token
+                if (i + 1 < tokenCount && tokens[i + 1].type != JSMN_OBJECT) {
+                    i++; // Skip the value associated with this field
+                }
+                continue;
+            } else if (pToken->type == JSMN_OBJECT) {
+                if (configCount + 1 > maxIceConfigs) {
+                    // That's all we have room to parse
+                    break;
+                }
                 configCount++;
             } else if (compareJsonString(pResponseStr, pToken, JSMN_STRING, (PCHAR) "Username")) {
                 strLen = (UINT32) (pToken[1].end - pToken[1].start);
                 CHK(strLen <= MAX_ICE_CONFIG_USER_NAME_LEN, STATUS_INVALID_API_CALL_RETURN_JSON);
-                STRNCPY(pSignalingClient->iceConfigs[configCount - 1].userName, pResponseStr + pToken[1].start, strLen);
-                pSignalingClient->iceConfigs[configCount - 1].userName[MAX_ICE_CONFIG_USER_NAME_LEN] = '\0';
+                SNPRINTF(pIceConfigs[configCount - 1].userName, MAX_ICE_CONFIG_USER_NAME_BUFFER_LEN, "%.*s", strLen, pResponseStr + pToken[1].start);
                 i++;
             } else if (compareJsonString(pResponseStr, pToken, JSMN_STRING, (PCHAR) "Password")) {
                 strLen = (UINT32) (pToken[1].end - pToken[1].start);
                 CHK(strLen <= MAX_ICE_CONFIG_CREDENTIAL_LEN, STATUS_INVALID_API_CALL_RETURN_JSON);
-                STRNCPY(pSignalingClient->iceConfigs[configCount - 1].password, pResponseStr + pToken[1].start, strLen);
-                pSignalingClient->iceConfigs[configCount - 1].userName[MAX_ICE_CONFIG_CREDENTIAL_LEN] = '\0';
+                SNPRINTF(pIceConfigs[configCount - 1].password, MAX_ICE_CONFIG_CREDENTIAL_BUFFER_LEN, "%.*s", strLen, pResponseStr + pToken[1].start);
                 i++;
             } else if (compareJsonString(pResponseStr, pToken, JSMN_STRING, (PCHAR) "Ttl")) {
-                CHK_STATUS(STRTOUI64(pResponseStr + pToken[1].start, pResponseStr + pToken[1].end, 10, &ttl));
-
-                // NOTE: Ttl value is in seconds
-                pSignalingClient->iceConfigs[configCount - 1].ttl = ttl * HUNDREDS_OF_NANOS_IN_A_SECOND;
+                retStatus = STRTOUI64((PCHAR) pResponseStr + pToken[1].start, (PCHAR) pResponseStr + pToken[1].end, 10, &ttl);
+                if (STATUS_FAILED(retStatus)) {
+                    strLen = (UINT32) (pToken[1].end - pToken[1].start);
+                    DLOGE("Unable to convert TTL: %.*s to a number", strLen, (PCHAR) pResponseStr + pToken[1].start);
+                    retStatus = STATUS_INVALID_API_CALL_RETURN_JSON;
+                    CHK(FALSE, retStatus);
+                }
+                pIceConfigs[configCount - 1].ttl = ttl * HUNDREDS_OF_NANOS_IN_A_SECOND;
                 i++;
             } else if (compareJsonString(pResponseStr, pToken, JSMN_STRING, (PCHAR) "Uris")) {
-                // Expect an array of elements
                 CHK(pToken[1].type == JSMN_ARRAY, STATUS_INVALID_API_CALL_RETURN_JSON);
                 CHK(pToken[1].size <= MAX_ICE_CONFIG_URI_COUNT, STATUS_SIGNALING_MAX_ICE_URI_COUNT);
                 for (j = 0; j < pToken[1].size; j++) {
                     strLen = (UINT32) (pToken[j + 2].end - pToken[j + 2].start);
                     CHK(strLen <= MAX_ICE_CONFIG_URI_LEN, STATUS_SIGNALING_MAX_ICE_URI_LEN);
-                    STRNCPY(pSignalingClient->iceConfigs[configCount - 1].uris[j], pResponseStr + pToken[j + 2].start, strLen);
-                    pSignalingClient->iceConfigs[configCount - 1].uris[j][MAX_ICE_CONFIG_URI_LEN] = '\0';
-                    pSignalingClient->iceConfigs[configCount - 1].uriCount++;
+                    SNPRINTF(pIceConfigs[configCount - 1].uris[j], MAX_ICE_CONFIG_URI_BUFFER_LEN, "%.*s", strLen, pResponseStr + pToken[j + 2].start);
+                    pIceConfigs[configCount - 1].uriCount++;
                 }
-
                 i += pToken[1].size + 1;
             }
         }
     }
 
-    // Perform some validation on the ice configuration
-    pSignalingClient->iceConfigCount = configCount;
-    CHK_STATUS(validateIceConfiguration(pSignalingClient));
+    *pIceConfigCount = configCount;
 
 CleanUp:
-
-    if (STATUS_FAILED(retStatus)) {
-        DLOGE("Call Failed with Status:  0x%08x", retStatus);
-    }
-
-    freeLwsCallInfo(&pLwsCallInfo);
+    CHK_LOG_ERR(retStatus);
 
     LEAVES();
     return retStatus;
diff --git a/src/source/Signaling/LwsApiCalls.h b/src/source/Signaling/LwsApiCalls.h
@@ -281,6 +281,25 @@ STATUS wakeLwsServiceEventLoop(PSignalingClient, UINT32);
 STATUS terminateConnectionWithStatus(PSignalingClient, SERVICE_CALL_RESULT);
 STATUS configureLwsLogging(UINT32 kvsLogLevel);
 
+/**
+ * Parses ICE configuration from a JSON response string.
+ * If there are more ICE configurations in the string than maxIceConfigs, we will only
+ * parse up until maxIceConfigs.
+ *
+ * @param[in] pResponseStr JSON string containing ICE server configuration.
+ * @param[in] responseLen Length of the JSON string (excluding null-terminator).
+ * @param[in] maxIceConfigs Maximum number of ICE configurations the array can hold.
+ * @param[out] pIceConfigs Pointer to array of IceConfigInfo structures to be populated.
+ * @param[out] pIceConfigCount Pointer to receive the number of ICE configurations parsed.
+ *
+ * @return STATUS code of the execution:
+ * - STATUS_SUCCESS: Successfully parsed ICE configuration.
+ * - STATUS_NULL_ARG: Invalid NULL argument provided.
+ * - STATUS_INVALID_API_CALL_RETURN_JSON: Malformed JSON or missing required fields.
+ * - STATUS_SIGNALING_MAX_ICE_URI_COUNT: Too many URIs in configuration (more than MAX_ICE_CONFIG_URI_COUNT).
+ */
+STATUS parseIceConfigResponse(PCHAR, UINT32, UINT8, PIceConfigInfo, PUINT32);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/tst/IceConfigParsingTest.cpp b/tst/IceConfigParsingTest.cpp
