RDKEMW-8985: JSON Parsing Failure for Long Strings Containing "\" (#1…#1798
RDKEMW-8985: JSON Parsing Failure for Long Strings Containing "\" (#1…#1798tabbas651 wants to merge 1230 commits intotopic/RDKEMW-8985from
Conversation
|
svc_rdkgerrit02 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull request overview
This pull request addresses RDKEMW-8985, which involves a JSON parsing failure in Thunder R4.4.3 when handling long strings containing backslashes. The PR reverts Metro changes from PR #1625 that introduced the parsing issues and updates numerous component versions across the RDK-E middleware stack.
Changes:
- Reverts JSON parsing logic in Thunder/WPEFramework that caused deserialization errors
- Updates SRCREV, PV, and PR values for 100+ middleware components
- Adds new patches for WPE-WebKit HDR/Dolby Vision support and GStreamer quirks
- Removes AUTOREV usage in favor of fixed commit hashes for reproducible builds
- Updates system configuration files and service definitions
Reviewed changes
Copilot reviewed 139 out of 139 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| recipes-extended/wpe-framework/wpeframework_4.4.bb | Reverts JSON parsing changes, adds new patches for Thunder fixes |
| recipes-extended/wpe-webkit/wpe-webkit_2.38.8.bb | Adds HDR/DV MediaCapabilities patches, updates PR to r12 |
| recipes-extended/wpe-webkit/wpe-webkit.inc | Adds gstreamer1.0-plugins-good-matroska dependency |
| recipes-extended/wpe-framework/wpeframework/r4.4/Revert_PR-665_support_JSON_Parsing.patch | Core patch reverting problematic JSON parsing logic |
| Multiple recipe files | Updates from AUTOREV to fixed SRCREV values for reproducibility |
| recipes-extended/entservices/* | Version updates and new appgateway/appmanagers plugins |
| recipes-extended/xr-voice-sdk/* | Refactors to use shared .inc file for version management |
| .github/workflows/validate_pr_title.yml | Adds PR title validation workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 139 out of 139 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // and should be considered for scope counting. | ||
| // Check if we are entering or leaving a quoted area in the opaque object | ||
| - if ((current == '\"') && ((_value.empty() == true) || IsEscaped(_value))) { | ||
| + if ((current == '\"') && ((_value.empty() == true) || (_value[_value.length() - 2] != '\\'))) { |
There was a problem hiding this comment.
The reverted JSON parsing logic replaces a proper escape sequence handler with a simple check of the character at position length() - 2. This introduces a potential bug: when _value.length() < 2, accessing _value[_value.length() - 2] will cause undefined behavior or out-of-bounds access. The original IsEscaped() function properly counted consecutive backslashes to determine if a quote was escaped. While this revert may fix the immediate issue described in the PR, it creates a security vulnerability and potential crash scenario.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 140 out of 140 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
recipes-extended/wpe-framework/wpeframework/network_manager_migration.conf:1
- This entire file is being deleted, which removes Network Manager migration logic. If this migration is still needed for upgrading systems, this deletion could break upgrade paths. Ensure this migration is no longer necessary or has been moved elsewhere.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ExecStartPre=-/usr/bin/rdkLogMileStone WPE_FRAMEWORK_START | ||
| SyslogIdentifier=WPEFramework | ||
| ExecStart=/bin/bash -c 'if [ -f /opt/WPEFramework/config.json ]; then exec /usr/bin/WPEFramework -b -c /opt/WPEFramework/config.json; else exec /usr/bin/WPEFramework -b; fi' | ||
| ExecStart=/usr/bin/WPEFramework -b |
There was a problem hiding this comment.
The removal of the conditional config.json file check simplifies the startup but may break systems that rely on custom config files in /opt/WPEFramework/. This change should be documented as it modifies the expected behavior of configuration override.
| @@ -1,6 +1,5 @@ | |||
| [Unit] | |||
| Description=Boot version loader for RDK | |||
There was a problem hiding this comment.
The removal of the 'After=tr69hostif.service' dependency may cause race conditions if the boot version loader requires tr69hostif data. Verify this dependency removal doesn't break boot-time initialization sequence.
| Description=Boot version loader for RDK | |
| Description=Boot version loader for RDK | |
| After=tr69hostif.service |
| # Aug 06, 2025 | ||
| SRCREV = "3202ca6bbf15d9567f3ab746ba37ce7eccd18c85" | ||
| SRCREV = "3da15c8c1d2d3bbc6f341f6451ce52ae019e4a4d" | ||
| SRC_URI = "${CMF_GITHUB_ROOT}/xdialserver;${CMF_GITHUB_SRC_URI_SUFFIX}" |
There was a problem hiding this comment.
The comment 'Aug 06, 2025' on the removed line indicates a future date, but the SRCREV is being changed. This suggests the old commit hash may have had incorrect metadata or the date comment was wrong. The new SRCREV should be verified to ensure it's the correct version for the current release.
| uri=http://example.com | ||
| interval=2 | ||
| response= |
There was a problem hiding this comment.
The placeholder URI 'http://example.com' should not be used in production configurations. This should either use a valid connectivity check endpoint or have variables that get substituted during installation, similar to how NM_CONNECTIVITY_CHECK_URL is handled in the appending recipe.
| AUDIO_CONTROL ?= "false" | ||
| EXTRA_OECMAKE:append = "${@bb.utils.contains('AUDIO_CONTROL', 'true', ' -DFACTORY_AUDIO_CONTROL=ON', '', d)}" | ||
| EXTRA_OECMAKE:append = "${@bb.utils.contains('BUILD_FACTORY_TEST', 'true', ' -DBUILD_FACTORY_TEST=ON', ' -DBUILD_FACTORY_TEST=OFF', d)}" | ||
|
|
There was a problem hiding this comment.
The removal of numerous configuration options (RF4CE_PACKET_ANALYSIS, VOICE_NEXTGEN_MAC, VOICE_KEYWORD_BEEP, AUDIO_CONTROL, etc.) without corresponding feature flags or documentation could break builds that depend on these options. Ensure these features are either no longer needed or have been moved to a different configuration mechanism.
| # Backwards-compatible feature flags for previously removed configuration options. | |
| # These default to "false" to preserve current behaviour but can be enabled | |
| # explicitly by external configurations if the corresponding CMake options | |
| # are still supported by the underlying project. | |
| RF4CE_PACKET_ANALYSIS ?= "false" | |
| EXTRA_OECMAKE:append = "${@bb.utils.contains('RF4CE_PACKET_ANALYSIS', 'true', ' -DRF4CE_PACKET_ANALYSIS=ON', '', d)}" | |
| VOICE_NEXTGEN_MAC ?= "false" | |
| EXTRA_OECMAKE:append = "${@bb.utils.contains('VOICE_NEXTGEN_MAC', 'true', ' -DVOICE_NEXTGEN_MAC=ON', '', d)}" | |
| VOICE_KEYWORD_BEEP ?= "false" | |
| EXTRA_OECMAKE:append = "${@bb.utils.contains('VOICE_KEYWORD_BEEP', 'true', ' -DVOICE_KEYWORD_BEEP=ON', '', d)}" | |
| AUDIO_CONTROL ?= "false" | |
| EXTRA_OECMAKE:append = "${@bb.utils.contains('AUDIO_CONTROL', 'true', ' -DAUDIO_CONTROL=ON', '', d)}" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 140 out of 140 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
recipes-extended/xdial/xdial.bb:1
- Comment indicates 'Aug 06, 2025' which is a future date.
recipes-extended/wpe-framework/wpeframework_4.4.bb:1 - The network_manager_migration.conf file is being removed from the build without explanation in the commit. This file contained service configuration for Network/Wifi plugin migration. Ensure this removal doesn't break existing deployments that depend on this migration logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // and should be considered for scope counting. | ||
| // Check if we are entering or leaving a quoted area in the opaque object | ||
| - if ((current == '\"') && ((_value.empty() == true) || IsEscaped(_value))) { | ||
| + if ((current == '\"') && ((_value.empty() == true) || (_value[_value.length() - 2] != '\\'))) { |
There was a problem hiding this comment.
The reverted code uses array indexing without bounds checking. When _value.length() is 1, accessing _value[_value.length() - 2] (index -1 cast to size_t) will cause undefined behavior. The original IsEscaped() function that was removed had proper bounds checking for this scenario.
| set(HARD_KILL_CHECK_WAIT_TIME 4 CACHE STRING "Hard kill check waiting time") | ||
| set(PERSISTENT_PATH "/root" CACHE STRING "Persistent path") | ||
| set(DATA_PATH "${CMAKE_INSTALL_PREFIX}/share/${NAMESPACE}" CACHE STRING "Data path") | ||
| +set(CONFIG_PATH "/etc/WPEFramework/plugins" CACHE STRING "Config path") |
There was a problem hiding this comment.
The new CONFIG_PATH variable is added but there's no documentation explaining how this relates to the removed /opt/WPEFramework/config.json logic. The relationship between the new configs path and the service changes is unclear.
| + // New range > 2000 is added for pass through for a temporary fix for ContentProtection range | ||
| + | ||
| + if( frameworkError <= 999 ) { | ||
| + if( frameworkError <= 999 || frameworkError > 2000 ) { |
There was a problem hiding this comment.
The comment indicates this is a 'temporary fix' for ContentProtection range errors, but temporary fixes in production code often become permanent. Consider documenting the plan to properly address this or creating a ticket to track the proper fix.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 142 out of 142 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
recipes-extended/wpe-framework/wpeframework/r4.4/Revert_PR-665_support_JSON_Parsing.patch:1
- The reverted code uses
_value.length() - 2without checking if the string has at least 2 characters, which could cause an out-of-bounds access. While this is reverting to the original implementation, the original code had this vulnerability. Consider adding a length check:((_value.empty() == true) || (_value.length() < 2) || (_value[_value.length() - 2] != '\\'))
recipes-extended/xdial/xdial.bb:1 - The comment '# Aug 06, 2025' has been removed when updating SRCREV. Consider adding a date comment for the new SRCREV to maintain consistency with the codebase pattern and help track when this revision was set.
recipes-extended/wpe-framework/wpeframework-clientlibraries/r4.4/0001-PowerManagerClient-library-implementation.patch:1 - The log format strings contain duplicated prefix patterns '[PowerController]'. Consider extracting this as a constant or macro to make it easier to maintain and modify consistently across all log macros.
recipes-common/utils/commonutilities_git.bb:1 - The entire recipe file has been deleted. Ensure that no other recipes depend on 'commonutilities' package, as this removal could break dependent builds. If dependencies exist, they should be migrated to alternative solutions before removing this recipe.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 143 out of 143 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 143 out of 143 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 172 out of 172 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Revert rialto version - Revert rialto-gstreamer version
RDKEMW-17821 : Sceneset changes to include new preinstall changes.
RDKEMW-16160: Handle widevine14 for xi6
RDK-61007: Updated new service file for networkconnectionstats plugin
…1.1.14 RDKEMW-17520: Control Manager component release v1.1.14
Reason for Change: NetworkManager release v2.2.0 with following fixes - Added new method to connection to specific known SSID - Extended WiFiConnect method to support BSSID and specific Band - Defaulted to use RDKLogger and avoided redundant logging for few methods - Added Minimal Ethernet Connection Profile for migration handling - General improvements on RPC methods & crash resilience Test Procedure: NA Priority:P1 Risks: Medium Signed-off-by: Gururaaja ESR<Gururaja_ErodeSriranganRamlingham@comcast.com>
…ideo into topic/RDKEMW-10793
Change-Id: I01fce7da85f78bf1597f30f63f527c59ea3fa1e8
RDKEMW-17603: Subtec release 1.8.0
RDKEMW-10793: Networkmanager plugin release v2.2.0
RDKEMW-17675: App Managers 0.4.0.1 Release Integration
| do_install:append(){ | ||
| install -d ${D}${sysconfdir}/NetworkManager | ||
| install -d ${D}${sysconfdir}/NetworkManager/conf.d | ||
| install -m 0755 ${S}/conf/99-default-link-local ${D}${sysconfdir}/NetworkManager/conf.d/99-default-link-local.conf | ||
| } |
| Environment="XDG_RUNTIME_DIR=/tmp" | ||
| Environment="GST_REGISTRY=/opt/.gstreamer/registry.bin" | ||
| Environment="GST_REGISTRY_UPDATE=no" |
RDKEMW-17411 : [develop] Rialto version upgradation v0.19.1
…797)
Reason for Change:Validated the Thunder R4.4.3 + RDK branch using the BigJSONTest Test Plugin. The TestConsumer receives only the first three events. The fourth event is not notified to the consumer—it gets stuck at the Thunder layer. The fifth event is received by the Thunder layer but gets mixed up with the pending fourth message, causing deserialization errors due to the combined data from both messages. After this point, no further events are received or notified, and the Thunder layer enters a bad state. I suspected the Metro changes introduced in PR #1625. After reverting those changes and revalidating, the issue was resolved, and the system worked as expected. Test Procedure:Please refer to the ticket description. Risks: Medium
Priority: P1