-
Notifications
You must be signed in to change notification settings - Fork 10
RDKB-62810,RDKB-62812:initialize DHCP for Voice interface #53
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
Conversation
Reason for change:Added the new entry for virual mta interface Test Procedure: verify the logic and functionality Risks: Low Priority: P1
Reason for change:Added the new entry for virual mta interface Test Procedure: verify the logic and functionality Risks: Low Priority: P1
|
Is this a Bug or a User Story (US)? If it is a User Story: Please list all dependent PRs from other components, if any. The commit message must include both the User Story ticket and the Subtask ticket --> addressed All changes related to the User Story must be squashed and merged in a single commit. Do not raise pull requests for partial User Story changes. Has the code development for the User Story been completed? --> Not yet If yes, please share the Gerrit topic or list all dependent PRs across components, including any meta-layer changes. If yes, please share the links to the validation comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Reason for change:preparing custom options for 43 and 60 Test Procedure: verify the logic and functionality Risks: Low Priority: P1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 20 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| static int prepareDhcpOption43(char *pOptionValue, size_t iOptionValueSize) | ||
| { | ||
| srand((unsigned int)time(NULL)); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The srand function is called with time(NULL), but this function should typically be called once at program startup, not every time prepareDhcpOption43 is invoked. Calling srand repeatedly can reduce the randomness quality of subsequent rand() calls. Consider removing this call if srand is already initialized elsewhere in the application, or document why it's necessary here.
| srand((unsigned int)time(NULL)); |
| /* Suboption 24: multiGrants */ | ||
| cHexBuf[iIndex++] = 0x18; | ||
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.multiGrants); | ||
| cHexBuf[iIndex++] = sVoicePktcCapabilities.multiGrants; | ||
|
|
||
| /* Suboption 25: v_152 */ | ||
| cHexBuf[iIndex++] = 0x19; | ||
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.v_152);; | ||
| cHexBuf[iIndex++] = sVoicePktcCapabilities.v_152; | ||
|
|
||
| /* Suboption 26: certBootstrapping */ | ||
| cHexBuf[iIndex++] = 0x1A; | ||
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.certBootstrapping); | ||
| cHexBuf[iIndex++] = sVoicePktcCapabilities.certBootstrapping; | ||
|
|
||
| /* Suboption 38: ipAddrProvCap */ |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar buffer overflow risk: The cHexBuf array (size 256) in prepareDhcpOption60 has no bounds checking before writing. The function builds suboptions with data from voice capabilities, but never verifies that iIndex stays within the 256-byte limit. Add bounds checking before each write operation to prevent buffer overflow.
| /* Suboption 24: multiGrants */ | |
| cHexBuf[iIndex++] = 0x18; | |
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.multiGrants); | |
| cHexBuf[iIndex++] = sVoicePktcCapabilities.multiGrants; | |
| /* Suboption 25: v_152 */ | |
| cHexBuf[iIndex++] = 0x19; | |
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.v_152);; | |
| cHexBuf[iIndex++] = sVoicePktcCapabilities.v_152; | |
| /* Suboption 26: certBootstrapping */ | |
| cHexBuf[iIndex++] = 0x1A; | |
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.certBootstrapping); | |
| cHexBuf[iIndex++] = sVoicePktcCapabilities.certBootstrapping; | |
| /* Suboption 38: ipAddrProvCap */ | |
| /* Suboption 24: multiGrants */ | |
| if (iIndex + 3 > (int)sizeof(cHexBuf)) | |
| { | |
| DHCPMGR_LOG_ERROR("%s %d: cHexBuf too small for Suboption 24 (multiGrants)\n", __FUNCTION__, __LINE__); | |
| return -1; | |
| } | |
| cHexBuf[iIndex++] = 0x18; | |
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.multiGrants); | |
| cHexBuf[iIndex++] = sVoicePktcCapabilities.multiGrants; | |
| /* Suboption 25: v_152 */ | |
| if (iIndex + 3 > (int)sizeof(cHexBuf)) | |
| { | |
| DHCPMGR_LOG_ERROR("%s %d: cHexBuf too small for Suboption 25 (v_152)\n", __FUNCTION__, __LINE__); | |
| return -1; | |
| } | |
| cHexBuf[iIndex++] = 0x19; | |
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.v_152);; | |
| cHexBuf[iIndex++] = sVoicePktcCapabilities.v_152; | |
| /* Suboption 26: certBootstrapping */ | |
| if (iIndex + 3 > (int)sizeof(cHexBuf)) | |
| { | |
| DHCPMGR_LOG_ERROR("%s %d: cHexBuf too small for Suboption 26 (certBootstrapping)\n", __FUNCTION__, __LINE__); | |
| return -1; | |
| } | |
| cHexBuf[iIndex++] = 0x1A; | |
| cHexBuf[iIndex++] = sizeof(sVoicePktcCapabilities.certBootstrapping); | |
| cHexBuf[iIndex++] = sVoicePktcCapabilities.certBootstrapping; | |
| /* Suboption 38: ipAddrProvCap */ | |
| if (iIndex + 3 > (int)sizeof(cHexBuf)) | |
| { | |
| DHCPMGR_LOG_ERROR("%s %d: cHexBuf too small for Suboption 38 (ipAddrProvCap)\n", __FUNCTION__, __LINE__); | |
| return -1; | |
| } |
| static void readMacAddress (char * pMacAddress, int iMacBufSize) | ||
| { | ||
| if (pMacAddress == NULL || iMacBufSize <= 0) | ||
| { | ||
| DHCPMGR_LOG_ERROR("%s %d: Invalid args..\n", __FUNCTION__, __LINE__); | ||
| return; | ||
| } | ||
|
|
||
| FILE *pFILE = fopen("/tmp/factory_nvram.data", "r"); | ||
| if (pFILE != NULL) | ||
| { | ||
| char cLine[128] = {0}; | ||
| while (fgets(cLine, sizeof(cLine), pFILE) != NULL) | ||
| { | ||
| if (strncmp(cLine, "EMTA ",5) == 0) | ||
| { | ||
| char *pMac = cLine + 5; | ||
| pMac[strcspn(pMac, "\n")] = 0; // Remove newline character | ||
| snprintf(pMacAddress, iMacBufSize, "%s", pMac); | ||
| break; | ||
| } | ||
| } | ||
| fclose(pFILE); | ||
| } | ||
| else | ||
| { | ||
| CcspTraceError(("%s: Failed to open /tmp/factory_nvram.data\n", __FUNCTION__)); | ||
| } | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readMacAddress function doesn't clear or initialize pMacAddress before attempting to read. If the file doesn't contain an "EMTA " line, the function returns without modifying pMacAddress, leaving it in an undefined state. The caller might then use uninitialized data. The function should either clear the buffer at the start or explicitly set it to an empty string if no MAC address is found.
| yes) dhcp_voiceOptions_support=true ;; | ||
| no) dhcp_voiceOptions_support=false ;; | ||
| *) AC_MSG_ERROR([bad value ${enableval} for --enable-dhcpVoiceOptions_support]) ;; | ||
| esac],[dhcp_voiceOptions_support=false]) | ||
| AM_CONDITIONAL(DHCP_VOICE_OPTIONS_SUPPORT, test x"$dhcp_voiceOptions_support" = x"true") |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent variable naming: The configure option is named "dhcpVoiceOptions_support" in the AC_ARG_ENABLE but the internal variable is named "dhcp_voiceOptions_support" (note the underscore positions differ). This inconsistency could lead to confusion during debugging or when modifying the build system. The variable naming should be consistent throughout.
| yes) dhcp_voiceOptions_support=true ;; | |
| no) dhcp_voiceOptions_support=false ;; | |
| *) AC_MSG_ERROR([bad value ${enableval} for --enable-dhcpVoiceOptions_support]) ;; | |
| esac],[dhcp_voiceOptions_support=false]) | |
| AM_CONDITIONAL(DHCP_VOICE_OPTIONS_SUPPORT, test x"$dhcp_voiceOptions_support" = x"true") | |
| yes) dhcpVoiceOptions_support=true ;; | |
| no) dhcpVoiceOptions_support=false ;; | |
| *) AC_MSG_ERROR([bad value ${enableval} for --enable-dhcpVoiceOptions_support]) ;; | |
| esac],[dhcpVoiceOptions_support=false]) | |
| AM_CONDITIONAL(DHCP_VOICE_OPTIONS_SUPPORT, test x"$dhcpVoiceOptions_support" = x"true") |
| unsigned char cHexBuf[256] = {0}; | ||
| int iIndex = 0; | ||
|
|
||
| /* Vendor prefix (Technicolor/Comcast specific) */ |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vendor prefix bytes 0x05 and 0x49 are hardcoded without explanation. These magic numbers should be documented with comments explaining what they represent in the DHCP Option 60 specification. The comment mentions "Technicolor/Comcast specific" but doesn't explain the meaning of these specific bytes.
| /* Vendor prefix (Technicolor/Comcast specific) */ | |
| /* | |
| * Vendor prefix (Technicolor/Comcast specific). | |
| * The bytes 0x05 and 0x49 are the fixed vendor prefix defined by the | |
| * Technicolor/Comcast DHCP Option 60 / Option 43 specification for | |
| * voice provisioning. These values identify the vendor-specific | |
| * encoding and MUST NOT be changed, otherwise the DHCP server / | |
| * provisioning system will not recognize this option. | |
| */ |
| } | ||
| for (int iInd = 0; iInd < iIndex; iInd++) | ||
| { | ||
| int iRet = snprintf(pOptionValue + iWritten, iOptionValueSize - iWritten, "%02X", cHexBuf[iInd]); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prepareDhcpOption60 function writes the value in uppercase hex format (%02X) while prepareDhcpOption43 uses lowercase hex format (%02x). This inconsistency could cause issues if the DHCP server or client expects a specific case. The hex format should be consistent across both functions or the difference should be documented if intentional.
| int iRet = snprintf(pOptionValue + iWritten, iOptionValueSize - iWritten, "%02X", cHexBuf[iInd]); | |
| int iRet = snprintf(pOptionValue + iWritten, iOptionValueSize - iWritten, "%02x", cHexBuf[iInd]); |
|
|
||
| //Type 20: Correlation ID - Random 4 bytes | ||
| // Generate random correlation ID | ||
| uint32_t ui32CorrelationId = ((uint32_t)rand() << 16) | ((uint32_t)rand() & 0xFFFF); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using rand() for generating a correlation ID is not cryptographically secure and may produce predictable values. For network protocols and identifiers that might have security implications, consider using a more secure random number generator like getrandom() or reading from /dev/urandom.
| rbusError_t getDataHandler(rbusHandle_t rbusHandle, rbusProperty_t rbusProperty,rbusGetHandlerOptions_t *pRbusGetHandlerOptions) | ||
| { | ||
| (void)rbusHandle; | ||
| (void)pRbusGetHandlerOptions; | ||
| (void)rbusProperty; | ||
| return RBUS_ERROR_SUCCESS; | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getDataHandler function is a stub that does nothing but return success. This incomplete implementation could mask issues when other components request data for registered rbus properties. The function should either be properly implemented to return actual data, or if it's intentionally a placeholder, this should be documented with a TODO or FIXME comment explaining when it will be implemented.
| unsigned char cHexBuf[256] = {0}; | ||
| int iIndex = 0; | ||
|
|
||
| //Type 02: Vendor Name | ||
| cHexBuf[iIndex++] = 0x02; //Suboption Type | ||
| cHexBuf[iIndex++] = 0x04; //Length | ||
| cHexBuf[iIndex++] = 0x45; //'E' | ||
| cHexBuf[iIndex++] = 0x44; //'D' | ||
| cHexBuf[iIndex++] = 0x56; //'V' | ||
| cHexBuf[iIndex++] = 0x41; //'A' | ||
|
|
||
| DHCPMGR_LOG_INFO("%s %d: EVDA Identifier:%02x %02x %02x %02x\n", __FUNCTION__, __LINE__, | ||
| cHexBuf[iIndex-4], cHexBuf[iIndex-3], cHexBuf[iIndex-2], cHexBuf[iIndex-1]); | ||
|
|
||
| //Type 04: Serial Number | ||
| int iSerialNumLen = (int)strlen(sDhcpOption43RawData.cSerialNumber); | ||
| cHexBuf[iIndex++] = 0x04; //Suboption Type | ||
| cHexBuf[iIndex++] = (unsigned char)iSerialNumLen; //Length | ||
| memcpy(&cHexBuf[iIndex], sDhcpOption43RawData.cSerialNumber, iSerialNumLen); | ||
| iIndex += iSerialNumLen; | ||
|
|
||
| //Type 05: Hardware Version | ||
| int iHardwareVerLen = (int)strlen(sDhcpOption43RawData.cHardwareVersion); | ||
| cHexBuf[iIndex++] = 0x05; //Suboption Type | ||
| cHexBuf[iIndex++] = (unsigned char)iHardwareVerLen; //Length | ||
| memcpy(&cHexBuf[iIndex], sDhcpOption43RawData.cHardwareVersion, iHardwareVerLen); | ||
| iIndex += iHardwareVerLen; | ||
|
|
||
| //Type 06: Software Version | ||
| int iSoftwareVerLen = (int)strlen(sDhcpOption43RawData.cSoftwareVersion); | ||
| cHexBuf[iIndex++] = 0x06; //Suboption Type | ||
| cHexBuf[iIndex++] = (unsigned char)iSoftwareVerLen; //Length | ||
| memcpy(&cHexBuf[iIndex], sDhcpOption43RawData.cSoftwareVersion, iSoftwareVerLen); | ||
| iIndex += iSoftwareVerLen; | ||
|
|
||
| //Type 07: Bootloader Version | ||
| int iBootloaderVerLen = (int)strlen(sDhcpOption43RawData.cBootLoaderVersion); | ||
| cHexBuf[iIndex++] = 0x07; //Suboption Type | ||
| cHexBuf[iIndex++] = (unsigned char)iBootloaderVerLen; //Length | ||
| memcpy(&cHexBuf[iIndex], sDhcpOption43RawData.cBootLoaderVersion, iBootloaderVerLen); | ||
| iIndex += iBootloaderVerLen; | ||
|
|
||
| //Type 08: OUID | ||
| int iOUIDLen = 3; // As per dhcp option 60 type 08 OUI length is 3 bytes | ||
| cHexBuf[iIndex++] = 0x08; //Suboption Type | ||
| cHexBuf[iIndex++] = (unsigned char)iOUIDLen; //Length | ||
| memcpy(&cHexBuf[iIndex], sDhcpOption43RawData.cOUID, iOUIDLen); | ||
| iIndex += iOUIDLen; | ||
|
|
||
| //Type 09: Model Number | ||
| int iModelNumLen = (int)strlen(sDhcpOption43RawData.cModelNumber); | ||
| cHexBuf[iIndex++] = 0x09; //Suboption Type | ||
| cHexBuf[iIndex++] = (unsigned char)iModelNumLen; //Length | ||
| memcpy(&cHexBuf[iIndex], sDhcpOption43RawData.cModelNumber, iModelNumLen); | ||
| iIndex += iModelNumLen; | ||
|
|
||
| //Type 0A: Vendor Name | ||
| int iVendorNameLen = (int)strlen(sDhcpOption43RawData.cVendorName); | ||
| cHexBuf[iIndex++] = 0x0A; //Suboption Type | ||
| cHexBuf[iIndex++] = (unsigned char)iVendorNameLen; //Length | ||
| memcpy(&cHexBuf[iIndex], sDhcpOption43RawData.cVendorName, iVendorNameLen); | ||
| iIndex += iVendorNameLen; | ||
|
|
||
| //Type 1F: MTA MAC Address | ||
| cHexBuf[iIndex++] = 0x1F; //Suboption Type | ||
| cHexBuf[iIndex++] = 0x06; //As per dhcp option 60 type 1F MAC address length is 6 bytes | ||
| unsigned char cMtaMacAddress[6] = {0}; | ||
| if (sscanf(sDhcpOption43RawData.cMtaMacAddress, "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx", | ||
| &cMtaMacAddress[0], &cMtaMacAddress[1], &cMtaMacAddress[2], | ||
| &cMtaMacAddress[3], &cMtaMacAddress[4], &cMtaMacAddress[5]) == 6) | ||
| { | ||
| memcpy(&cHexBuf[iIndex], cMtaMacAddress, 6); | ||
| iIndex += 6; | ||
| } | ||
| else | ||
| { | ||
| if (sscanf(sDhcpOption43RawData.cMtaMacAddress, "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx", | ||
| &cMtaMacAddress[0], &cMtaMacAddress[1], &cMtaMacAddress[2], | ||
| &cMtaMacAddress[3], &cMtaMacAddress[4], &cMtaMacAddress[5]) == 6) | ||
| { | ||
| memcpy(&cHexBuf[iIndex], cMtaMacAddress, 6); | ||
| iIndex += 6; | ||
| } | ||
| else | ||
| { | ||
| DHCPMGR_LOG_ERROR("%s %d: Invalid MTA MAC Address format..\n", __FUNCTION__, __LINE__); | ||
| } | ||
| } | ||
|
|
||
| //Type 20: Correlation ID - Random 4 bytes | ||
| // Generate random correlation ID | ||
| uint32_t ui32CorrelationId = ((uint32_t)rand() << 16) | ((uint32_t)rand() & 0xFFFF); | ||
|
|
||
| cHexBuf[iIndex++] = 0x20; //Suboption Type | ||
| cHexBuf[iIndex++] = 0x04; //Length | ||
| cHexBuf[iIndex++] = (unsigned char)((ui32CorrelationId >> 24) & 0xFF); | ||
| cHexBuf[iIndex++] = (unsigned char)((ui32CorrelationId >> 16) & 0xFF); | ||
| cHexBuf[iIndex++] = (unsigned char)((ui32CorrelationId >> 8) & 0xFF); | ||
| cHexBuf[iIndex++] = (unsigned char)(ui32CorrelationId & 0xFF); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential buffer overflow risk: The cHexBuf array is declared with size 256, but there's no check to ensure iIndex doesn't exceed this size before writing to the buffer. If the combined length of all device information fields (serial number, hardware version, software version, bootloader version, vendor name, model number) exceeds the available space, this will cause a buffer overflow. Add a bounds check before each memcpy or array access to ensure iIndex stays within bounds.
| cHexBuf[iIndex++] = 0x56; //'V' | ||
| cHexBuf[iIndex++] = 0x41; //'A' | ||
|
|
||
| DHCPMGR_LOG_INFO("%s %d: EVDA Identifier:%02x %02x %02x %02x\n", __FUNCTION__, __LINE__, |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions "EVDA" but the actual identifier is "EDVA". This appears to be a typo in the log message.
| DHCPMGR_LOG_INFO("%s %d: EVDA Identifier:%02x %02x %02x %02x\n", __FUNCTION__, __LINE__, | |
| DHCPMGR_LOG_INFO("%s %d: EDVA Identifier:%02x %02x %02x %02x\n", __FUNCTION__, __LINE__, |
Reason for change:Adding the DHCP options 43 and 60 for Voice support
Test Procedure: Voice interface should get the IP via DHCP manager.
Risks: High
Priority: P1