Porting of fixes from master to R4_4#2052
Conversation
…/customerrorcodes
There was a problem hiding this comment.
Pull request overview
This PR backports multiple bug fixes and enhancements from the master branch to the R4_4 branch. The changes address critical issues including JSON parsing failures with escaped quotes, socket lifecycle race conditions, COM initialization timing, and introduces a new custom error code feature with configurable string conversion support.
Changes:
- Fixes JSON parsing bug with escaped quotes in strings (issue #1948)
- Adds exact key matching for JSON object Remove() operation
- Introduces custom error code framework with configurable library support
- Fixes socket port race condition in Closed() method and adds REMOTE_CLOSED state
- Moves DefaultInvokeServer to SingletonProxyType to fix Windows threading issues
- Adds comprehensive unit tests for JSON object operations
- Disables incomplete WebRequest assert temporarily (METROL-1211)
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/unit/core/test_jsonobject.cpp | New unit tests for JSON object Remove() operations and VariantContainer functionality |
| Tests/unit/core/CMakeLists.txt | Adds test_jsonobject.cpp to build |
| Tests/unit/Module.h | New module header for unit tests |
| Source/core/SocketPort.h | Adds REMOTE_CLOSED state for proper socket lifecycle tracking |
| Source/core/SocketPort.cpp | Reverts to original Closed() logic, adds state change propagation for REMOTE_CLOSED |
| Source/core/Singleton.h | Changes SingletonProxyType return type to reference and adds destructor assertion |
| Source/core/Proxy.h | Adds LastRefAccessor for debug purposes and ASSERT in ProxyType constructor |
| Source/core/Portability.h | Adds custom error code support with int24_t types and new warning suppression |
| Source/core/Portability.cpp | Implements custom error code conversion and handling functions |
| Source/core/JSONRPC.h | Updates error code handling to support custom codes |
| Source/core/JSON.h | Fixes IsEscaped() calculation and Remove() to use exact key match |
| Source/core/IPCConnector.h | Adds InProgress() method |
| Source/core/ICustomErrorCode.h | New interface for custom error code libraries |
| Source/core/CMakeLists.txt | Adds ICustomErrorCode.h to public headers |
| Source/com/IUnknown.h | Moves Invoke() declaration out of inline |
| Source/com/IUnknown.cpp | Implements Invoke() with deadlock detection |
| Source/com/ConnectorType.cpp | Uses SingletonProxyType for DefaultInvokeServer |
| Source/WPEFramework/PluginServer.h | Disables incomplete request ASSERT temporarily |
| Source/WPEFramework/PluginHost.cpp | Adds CustomCodeLibrary loading support |
| Source/WPEFramework/Config.h | Adds customcodelibrary configuration option |
| Source/CMakeLists.txt | Adds DISABLE_USE_COMPLEMENTARY_CODE_SET option |
💡 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 22 out of 22 changed files in this pull request and generated 3 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 23 out of 23 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
Source/core/JSON.h:3824
Container::RemovecomparesTCHAR*labels usingstrlen/strcmp, which breaks when_UNICODEis enabled (TCHAR becomes wchar_t) and also diverges from the existing codebase convention of using_tcslen/_tcscmpfor TCHAR strings. Use_tcslen/_tcscmp(or equivalent TCHAR-safe comparison) here.
void Remove(const TCHAR label[])
{
JSONElementList::iterator index(_data.begin());
while (index != _data.end() && (strlen(index->first) != strlen(label) || strcmp(index->first, label) != 0)) {
index++;
}
if (index != _data.end()) {
_data.erase(index);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Source/core/Portability.h
Outdated
| #define DISABLE_WARNING_UNUSED_PARAMETERS PUSH_WARNING_ARG_(4100) | ||
| // W4 - 'function': unreferenced function with internal linkage has been removed | ||
| #define DISABLE_WARNING_UNUSED_FUNCTIONS PUSH_WARNING_ARG_(5242) | ||
| / W3 - 'argument': conversion from 'type' to 'type', possible loss of data |
There was a problem hiding this comment.
Line 176 starts with a single '/' which is not a valid comment opener, so this will not compile on Windows. It should be changed to '//' (or a proper /* ... */ block) before defining DISABLE_WARNING_CONVERSION_POSSIBLE_LOSS_OF_DATA.
| / W3 - 'argument': conversion from 'type' to 'type', possible loss of data | |
| // W3 - 'argument': conversion from 'type' to 'type', possible loss of data |
There was a problem hiding this comment.
Suggestion is addressed
Source/core/JSONRPC.h
Outdated
| #endif | ||
| Text = Core::ErrorToString(frameworkError); |
There was a problem hiding this comment.
Text is conditionally set to Core::ErrorToStringExtended(frameworkError) but then is unconditionally overwritten by Core::ErrorToString(frameworkError) on the next line, so the extended message is never returned. Adjust the control flow so the extended string is preserved when intended (e.g., only fall back to ErrorToString when Text is still unset).
| #endif | |
| Text = Core::ErrorToString(frameworkError); | |
| #endif | |
| if (Text.IsSet() == false) { | |
| Text = Core::ErrorToString(frameworkError); | |
| } |
| ::WPEFramework::Core::JSON::Variant doubleFound = container.Get("double"); | ||
| ::WPEFramework::Core::JSON::Variant& doubleRefFound = container["double"]; | ||
| EXPECT_TRUE(doubleFound.Content() == ::WPEFramework::Core::JSON::Variant::type::DOUBLE); | ||
| EXPECT_FLOAT_EQ(doubleFound.Float(), 123.456); |
There was a problem hiding this comment.
This test validates a DOUBLE Variant but compares it via doubleFound.Float() using EXPECT_FLOAT_EQ, which truncates the double to float and can hide precision/rounding issues. Use doubleFound.Double() with EXPECT_DOUBLE_EQ (or EXPECT_NEAR with a suitable tolerance) to actually test the double path.
| EXPECT_FLOAT_EQ(doubleFound.Float(), 123.456); | |
| EXPECT_DOUBLE_EQ(doubleFound.Double(), 123.456); |
|
@sivarajappan-siva I've opened a new pull request, #2071, to work on those changes. Once the pull request is ready, I'll request review from you. |
…_CODE_SET__ Co-authored-by: sivarajappan-siva <217732782+sivarajappan-siva@users.noreply.github.com>
* Fix RAM/swap reporting to use sysinfo mem_unit scaling Fix RAM/swap reporting to use sysinfo mem_unit scaling * Update SystemInfo.cpp --------- Co-authored-by: MFransen69 <39826971+MFransen69@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| bool IsEscaped(const string& value) const { | ||
| // This code determines if a lot of back slashes to esscape the backslash | ||
| // Is odd or even, so does it escape the last character.. | ||
| // e.g. 'Test \\\\\\\\\\"' is not the escaping of the quote (") | ||
| // 'Test \\\\\\\\\" continued"' is the escaping of th quote.. | ||
| // 'Test \" and \" and than \\\"' are all escaped quotes | ||
| uint32_t index = static_cast<uint32_t>(value.length() - 1); | ||
| uint32_t start = index; | ||
| while ( (index != static_cast<uint32_t>(~0)) && (value[index] == '\\') ) { | ||
| index--; | ||
| } | ||
| return (((start - index) % 2) == 0); | ||
| bool escaped(false); | ||
|
|
||
| if (_value.empty() == false) { | ||
| // This code determines if a lot of back slashes to esscape the backslash | ||
| // Is odd or even, so does it escape the last character.. | ||
| // e.g. 'Test \\\\\\\\\\"' is not the escaping of the quote (") | ||
| // 'Test \\\\\\\\\" continued"' is the escaping of th quote.. | ||
| // 'Test \" and \" and than \\\"' are all escaped quotes | ||
| // Subtracting 2 here, because the length is always counted by | ||
| // starting @1, so to a zero based buffer, i need to substract 1 | ||
| // however that will give us the last character. This is the character | ||
| // for which we would like to know if is is escaped, so I need to go | ||
| // even one position before that one, hence -2 | ||
| uint32_t index = static_cast<uint32_t>(value.length() - 2); | ||
| uint32_t count = 0; | ||
| while ((index != static_cast<uint32_t>(~0)) && (value[index] == '\\')) { |
There was a problem hiding this comment.
IsEscaped(const string& value) incorrectly checks _value.empty() instead of value.empty(), and it computes value.length() - 2 without guarding for value.length() < 2. When the first character in the opaque value is a quote, this underflows and will read out-of-bounds while scanning for backslashes. Use value consistently and add a length check before subtracting 2 (or restructure the algorithm to avoid unsigned underflow).
| void Remove(const TCHAR label[]) | ||
| { | ||
| JSONElementList::iterator index(_data.begin()); | ||
|
|
||
| while ((index != _data.end()) && (index->first != label)) { | ||
| while (index != _data.end() && (strlen(index->first) != strlen(label) || strcmp(index->first, label) != 0)) { | ||
| index++; | ||
| } |
There was a problem hiding this comment.
Container::Remove uses strlen/strcmp on const TCHAR* labels. This will not compile (or will behave incorrectly) when _UNICODE is enabled and TCHAR is wchar_t. Use _tcslen/_tcscmp (or equivalent std::char_traits<TCHAR> comparison) instead of narrow-string functions.
| if (customcode == std::numeric_limits<int32_t>::max()) { | ||
| text = _T("Invalid Custom ErrorCode set"); | ||
| } else if ((customerrorcodehandler == nullptr) || ((text = customerrorcodehandler(customcode)) == nullptr)) { | ||
| text = _T("Undefined Custom Error"); |
There was a problem hiding this comment.
This file uses std::numeric_limits<int32_t>::max() but does not include <limits> in its includes, which can break compilation depending on transitive includes. Add #include <limits> here (or in a header that guarantees it for this TU).
| #pragma once | ||
|
|
||
| #undef EXTERNAL | ||
|
|
||
| #if defined(WIN32) || defined(_WINDOWS) || defined(__CYGWIN__) || defined(_WIN64) | ||
| #define EXTERNAL __declspec(dllexport) | ||
| #else | ||
| #define EXTERNAL __attribute__((visibility("default"))) | ||
| #endif | ||
|
|
There was a problem hiding this comment.
This header undefines and redefines the global EXTERNAL macro, which can unexpectedly change symbol visibility/import-export semantics for any headers included after it in the same translation unit. Prefer a dedicated export macro for this interface (e.g. CUSTOM_ERRORCODE_EXPORT) or restore the previous EXTERNAL definition after this header to avoid macro pollution.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <stdlib.h> | ||
|
|
||
| #include "Module.h" | ||
| #include "IPCConnector.h" | ||
| #include "Portability.h" | ||
| #include "Sync.h" | ||
| #include "SystemInfo.h" | ||
| #include "Serialization.h" | ||
| #include "Number.h" | ||
|
|
There was a problem hiding this comment.
This file uses std::numeric_limits but does not include <limits>. Relying on transitive includes is non-portable and can fail to compile depending on toolchain/standard library headers. Add an explicit #include <limits> here (or in a guaranteed common header) to make this translation unit self-contained.
| m_syncAdmin.Lock(); | ||
|
|
||
| // Turn them all off, except for the SHUTDOWN bit, to show whether this was | ||
| // done on our request, or closed from the other side... | ||
| m_State &= SHUTDOWN; | ||
|
|
||
| StateChange(); | ||
|
|
||
| m_State &= (~SHUTDOWN); | ||
| DestroySocket(m_Socket); | ||
| ResourceMonitor::Instance().Unregister(*this); | ||
|
|
There was a problem hiding this comment.
SocketPort::Closed() now does m_State &= SHUTDOWN; which can set m_State to 0 for remote-initiated closes (SHUTDOWN bit not set). Other threads (e.g. SocketServer cleanup using IsClosed()) can observe State()==0 and destroy the socket object while Closed() is still executing, reintroducing the use-after-free race described in the PR context. Avoid making State()==0 until after the ResourceMonitor thread is completely done with the object (e.g., preserve the MONITOR bit while in Closed(), or take a self-reference/other lifetime guard until after the final state transition).
| bool IsEscaped(const string& value) const { | ||
| // This code determines if a lot of back slashes to esscape the backslash | ||
| // Is odd or even, so does it escape the last character.. | ||
| // e.g. 'Test \\\\\\\\\\"' is not the escaping of the quote (") | ||
| // 'Test \\\\\\\\\" continued"' is the escaping of th quote.. | ||
| // 'Test \" and \" and than \\\"' are all escaped quotes | ||
| uint32_t index = static_cast<uint32_t>(value.length() - 1); | ||
| uint32_t start = index; | ||
| while ( (index != static_cast<uint32_t>(~0)) && (value[index] == '\\') ) { | ||
| index--; | ||
| } | ||
| return (((start - index) % 2) == 0); | ||
| bool escaped(false); | ||
|
|
||
| if (_value.empty() == false) { | ||
| // This code determines if a lot of back slashes to esscape the backslash | ||
| // Is odd or even, so does it escape the last character.. | ||
| // e.g. 'Test \\\\\\\\\\"' is not the escaping of the quote (") | ||
| // 'Test \\\\\\\\\" continued"' is the escaping of th quote.. | ||
| // 'Test \" and \" and than \\\"' are all escaped quotes | ||
| // Subtracting 2 here, because the length is always counted by | ||
| // starting @1, so to a zero based buffer, i need to substract 1 | ||
| // however that will give us the last character. This is the character | ||
| // for which we would like to know if is is escaped, so I need to go | ||
| // even one position before that one, hence -2 | ||
| uint32_t index = static_cast<uint32_t>(value.length() - 2); | ||
| uint32_t count = 0; | ||
| while ((index != static_cast<uint32_t>(~0)) && (value[index] == '\\')) { | ||
| index--; | ||
| count++; | ||
| } | ||
| escaped = ((count % 2) != 0); | ||
| } | ||
| return (escaped); |
There was a problem hiding this comment.
IsEscaped(const string& value) checks _value.empty() instead of the value parameter, and then unconditionally computes value.length() - 2. For 1-character strings (e.g. parsing "a"), this underflows and can read out of bounds, potentially crashing the JSON parser. Use the value parameter for emptiness/length checks and handle value.size() < 2 as not-escaped (or equivalent safe behavior).
| if ((code & CUSTOM_ERROR) != 0) { | ||
| result = static_cast<uint32_t>(ToInt24_Truncate(code)); // remove custom error bit before assigning | ||
| if (result == 0) { | ||
| result = std::numeric_limits<int32_t>::max(); // this will assert in debug, but if that happens one managed to fill an hresult with an overflowed core result, that should have asserted already when using CustomCode to fill it, os this is probably caused by either manually incorrectly filling the hresult or memory corruption | ||
| } | ||
| } |
There was a problem hiding this comment.
In IsCustomCode, result = static_cast<uint32_t>(ToInt24_Truncate(code)); converts a potentially negative 24-bit value to uint32_t and then back to int24_t (alias of int32_t). That roundtrip is implementation-defined for negative values and can break custom error decoding on some platforms. Assign the int32_t result of ToInt24_Truncate directly to int24_t (no unsigned cast) or use a well-defined bit-cast approach.
• [Core] another typo in the header file... (#2024)
• [Core] Type in comment (#2021)
• Starting COM after Controller is initialized (#2001)
• Break dependency of UnknownProxy::Invoke (#1999)
• [Core] disable webrequest incomplete assert for now (#1992)
• [LIMIT] Limit handing out interfaces of Plugins only if the plugin is active! (#1977)
• [FIX] for issue #1948 (#1976)