Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Processing invalid PT after cutting off unknown_parameter or unknown_RPC #1921

Closed
9 tasks done
GetmanetsIrina opened this issue Nov 24, 2017 · 11 comments
Closed
9 tasks done
Assignees

Comments

@GetmanetsIrina
Copy link

GetmanetsIrina commented Nov 24, 2017

Occurrence: Always

Steps to reproduce

  1. Make sure SDL is built with default flags
  2. Start SDL and HMI
  3. Connect mobile device
  4. Register new application
  5. Create PT for policy table update with unknown_parameter or unknown_RPC
  6. Perform policy table update from mobile app with created PT

Expected result

  1. SDL received UpdatedPT with at least one <unknown_parameter> or <unknown_RPC>
    and after cutting off <unknown_parameter> or <unknown_RPC> UpdatedPT performs validation of received PT
  2. SDL must log the error internally and discard Policy Table Update

Actual result

SDL performs PT validation without cutting off <unknown_parameter> or <unknown_RPC>

Test script

1921_Invalid_PT_after_cutting_unknow_values.lua

Environment

Attachments

Expected delivery

  • Source code updates
  • Code comments
  • UTs add/update (not required)
  • ATF tests add/update
  • Manual tests (not required)
  • Add/update CI plans/jobs (not required)
  • SDD updates (not required)
  • Guidelines update (sdl_core_guides) (not required)
  • Guidelines update (sdl_hmi_integration_guidelines) (not required)
@dboltovskyi
Copy link
Contributor

Priority was set to High. Updating policy with invalid PTU (which contains unknown RPC or parameter) may affect application permissions calculating process.

@GetmanetsIrina
Copy link
Author

GetmanetsIrina commented Dec 27, 2017

Issue is reproduced on develop branch (0b19cf4).

@LuxoftAKutsan
Copy link
Contributor

Issue is related to #1885

@LuxoftAKutsan
Copy link
Contributor

LuxoftAKutsan commented Dec 27, 2017

Fix is available #1972

But fix is partial. It includes only cutting off <unknown_parameter> And It does not cut's of <unknown_RPC>.

Missing functionality of cutting <unknown_RPC> is nice to have but there are no use cases when present <unknown_RPC> could break the flow.
In case of Mobile application will send RPC with unknown FunctionID, it will be rejected earlier during parsing, and policy logic won't be involved. Locating of <unknown_RPC> in PolicyTable does not affect other RPCs in any use cases.

@robinmk, @mrapitis please considering this points, should cutting <unknown_RPC> functionality be implemented or not?

@robinmk
Copy link

robinmk commented Dec 28, 2017

@LuxoftAKutsan , I have the same understanding as well - that unknown RPCs will be rejected/ignored early on and hence it would not break policies. If there is any change in this understanding please inform us as we will need to address at the earliest.
Let's also wait for @mrapitis to comment.

@mrapitis
Copy link
Contributor

mrapitis commented Jan 2, 2018

@LuxoftAKutsan I agree with @robinmk comment as well -- the RPC should send a generic response back to the mobile if it is of an unknown type (as it does today) which should not impact policies.

@mrapitis
Copy link
Contributor

mrapitis commented May 25, 2018

@GetmanetsIrina are there any remaining tasks required here? It looks like we still have three items unchecked in the expected delivery section. Can you please check them if this was an oversight? The pr associated should be #1972

@GetmanetsIrina
Copy link
Author

@mrapitis , task status is updated, thanks! #1972 is mentioned above.

@LuxoftAKutsan
Copy link
Contributor

@mrapitis Sorry, the #2006 is the correct PR.

@mrapitis
Copy link
Contributor

@LuxoftAKutsan how to #2006 and #1972 differ? it looks like #1972 was already reviewed?

@LuxoftAKutsan
Copy link
Contributor

@mrapitis #2006 also reviewed, but not by zhimin.
I would say that #1972 is not ready and looks like a draft version. #2006 is the same approach but is more clear and finished. It is my fail that there was 2 PRs, #1972 should be closed. I will close it if you don't mind

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

7 participants