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

Fix/sdl 3620 SDL does not respond with NACK to start RPC service request with invalid data #3647

Conversation

VladSemenyuk
Copy link
Contributor

@VladSemenyuk VladSemenyuk commented Mar 3, 2021

Fixes #3620

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

ATF, Unit tests

Summary

Fix memory leak with session after Start RPC service NACK.
Fix OnServiceUpdate sending after Start RPC service with invalid data in bson payload for protected mode.
Fix NACK sending for Start any service with non-existent session id.

Bug Fixes

Start RPC service with invalid data in bson payload
not send StartServiceNack to mobile app
session added in SDL Connection handler(memory leak)

Start RPC service with not null session id(except for RPC start service for protected mode)
not send StartServiceNack to mobile app

Start any service with non-existent session id(existent session ids are returned in RPC StartServiceAck)
not send StartServiceNack to mobile app

Start RPC service with invalid data in bson payload for protected mode
not send 'OnServiceUpdate' notification to HMI with 'REQUEST_REJECTED'

Start RPC service for protected mode before HMI has provided BC.OnSystemTimeReady notification
not send 'OnServiceUpdate' notification to HMI with 'REQUEST_REJECTED'

CLA

…oad.

Fix memory leak with session after Start RPC service NACK.
Fix OnServiceUpdate sending after Start RPC service with invalid data in bson payload for protected mode.
Fix NACK sending for Start any service with non-existent session id.
@VladSemenyuk
Copy link
Contributor Author

@JackLivio this PR is ready for review

const SessionContext& context,
SessionContext& context,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately this is not allowed on a public method.

to change this parameter to non-const we need to mark this method as DEPRECATED and define a new method with the new signature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iCollin sorry for that. I declared a new function for that and deprecated the old one.
Note, that I have to make some extra tricks with casts in unit tests to avoid ambiguous call errors.
Fixed in 77231fc

@jordynmackool
Copy link

@VladSemenyuk please see the comment above. Thanks!

…on payload. Fix memory leak with session after Start RPC service NACK. Fix OnServiceUpdate sending after Start RPC service with invalid data in bson payload for protected mode. Fix NACK sending for Start any service with non-existent session id.
@AKalinich-Luxoft
Copy link
Contributor

@VladSemenyuk please see the comment above. Thanks!

@jordynmackool comments have been processed here

Comment on lines 2135 to 2136
NotifySessionStarted(
const_cast<SessionContext&>(context), rejected_params, err_reason);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear this will cause undefined behavior if called with a const context.

We need to preserve the old functionality (of calling with a const context) until a major version bump.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iCollin agree, it's unsafe code. I replaced this cast with the local copy of the same context. As const version of deprecated function is not supposed to change the context, but supposed to use the same callbacks, I believe we can work in this way - 58fb660
Note, that deprecated function is unused for now.

…a in bson payload. Fix memory leak with session after Start RPC service NACK. Fix OnServiceUpdate sending after Start RPC service with invalid data in bson payload for protected mode. Fix NACK sending for Start any service with non-existent session id.
@AKalinich-Luxoft AKalinich-Luxoft changed the base branch from develop to release/7.1.0-RC1 March 24, 2021 19:24
@iCollin iCollin merged commit 7219fa4 into smartdevicelink:release/7.1.0-RC1 Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants