Skip to content
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

R2: Fix JSONRPC Version() bug #1757

Open
wants to merge 1 commit into
base: R2
Choose a base branch
from

Conversation

risingPhil
Copy link

@risingPhil risingPhil commented Sep 20, 2024

Problem:

When following the example code described here:
https://rdkcentral.github.io/rdkservices/#/userguide/native?id=creating-native-clients

If I perform the "DeviceInfo.defaultresolution" request with callsign "DeviceInfo", I'm getting an error:
"Requested version is not supported".

But I also get this when trying other rdkservices APIs (for example: org.rdk.Bluetooth.getApiVersionNumber)

Root cause:

WPEFramework sends out the request like this:

{"jsonrpc":"2.0","id":1,"method":"DeviceInfo.0.defaultresolution","params":{}}

As you can see, it added a versionnumber '0' to the method parameter.

Bug:
When the example scenario is triggered as described above, WPEFramework::Core::JSONRPC::Version() is called with "DeviceInfo." as designator.

At the if-statement with the atoi statement, index remains at pos - 1 and therefore points to the 'o' character. Because index < pos, result would be set to the result of the atoi() call.

But as described here: https://cplusplus.com/reference/cstdlib/atoi/#google_vignette

If the string starts with an invalid integral number, atoi() will return 0.

Because the result of WPEFramework::Core::JSONRPC::Version() is no longer 0xFF, the _versionstring field of JONRPCLink gets set to ".0". This in turn causes it to get appended to the method field of the jsonrpc request.

Fix:

Make sure to add an isdigit() check before trying the atoi() call. If !isdigit(), the atoi() call is now being avoided.

The WPEFramework::Core::JSONRPC::Version() code has been changed in the master branch. But for RDK 6.0.0, the R2 branch of Thunder is still being used by default.

I therefore think it's likely this fix will only be needed for R2-based versions.

Problem:

When following the example code described here:
https://rdkcentral.github.io/rdkservices/#/userguide/native?id=creating-native-clients

If I perform the "DeviceInfo.defaultresolution" request with callsign "DeviceInfo", I'm getting an error:
 "Requested version is not supported".

But I also get this when trying other rdkservices APIs (for example: org.rdk.Bluetooth.getApiVersionNumber)

Root cause:

WPEFramework sends out the request like this:

{"jsonrpc":"2.0","id":1,"method":"DeviceInfo.0.defaultresolution","params":{}}

As you can see, it added a versionnumber '0' to the method parameter.

Bug:
When the example scenario is triggered as described above, WPEFramework::Core::JSONRPC::Version() is called
with "DeviceInfo." as designator.

At the if-statement with the atoi statement, index remains at pos - 1 and therefore points to the 'o' character.
Because index < pos, result would be set to the result of the atoi() call.

But as described here:
https://cplusplus.com/reference/cstdlib/atoi/#google_vignette

If the string starts with an invalid integral number, atoi() will return 0.

Because the result of WPEFramework::Core::JSONRPC::Version() is no longer 0xFF, the _versionstring field of JSONRPCLink
gets set to ".0". This in turn causes it to get appended to the method field of the jsonrpc request.

Fix:

Make sure to add an isdigit() check before trying the atoi() call. If !isdigit(), the atoi() call is now being avoided.

The WPEFramework::Core::JSONRPC::Version() code has been changed in the master branch. But for RDK 6.0.0,
the R2 branch of Thunder is still being used by default.

I therefore think it's likely this fix will only be needed for R2-based versions.
@risingPhil
Copy link
Author

By the way: here is a test application I wrote to test this scenario alongside a recipe.

You may have to edit the recipe to fit your personal folder path (externalsrc based). I have personally been building RDK using a docker container.

testapp.tar.gz

To use this test application from the terminal (over SSH on a raspberry pi):

rdkjsonrpc 127.0.0.1:9998 DeviceInfo defaultresolution

@CLAassistant
Copy link

CLAassistant commented Sep 20, 2024

CLA assistant check
All committers have signed the CLA.

@pwielders
Copy link
Contributor

@risingPhil good catch! Indeed on master the code is heavily changed but looks like there the check for IsDigit is done. Guess this is an R2 issue only!

@pwielders pwielders self-requested a review September 20, 2024 10:13
@pwielders pwielders added the R2 Pull request for R2 label Sep 20, 2024
@risingPhil
Copy link
Author

Thanks! ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R2 Pull request for R2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants