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.

Sign up for GitHub

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

Jump to bottom

[20306] Ignore 0x8007 if coming from other vendor #4291

Merged
merged 4 commits into from
Jan 25, 2024
Merged

[20306] Ignore 0x8007 if coming from other vendor #4291

merged 4 commits into from
Jan 25, 2024

Conversation

EduPonz
Copy link

@EduPonz EduPonz commented Jan 24, 2024 •
edited by MiguelCompany
Loading

Description

This PR fixes an interoperability issue introduced in #3733 occurring when a different vendor sends a DATA(p) which parameter list is to contain a parameter with PID 0x8007, which in Fast DDS is a custom PID (PID_NETWORK_CONFIGURATION_SET). When that happens, the deserialization in Fast DDS most likely fails or even worse gets a wrong interpretation.

Tested with CycloneDDS ShapesDemo and Fast DDS ShapesDemo (setting the auto_fill_type_information to false using eProsima/ShapesDemo#107).

@Mergifyio backport 2.12.x

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • N/A: Any new/modified methods have been properly documented using Doxygen.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • N/A: New feature has been added to the versions.md file (if applicable).
  • N/A: New feature has been documented/Current behavior is correctly described in the documentation.
  • Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@EduPonz EduPonz added this to the v2.13.2 milestone Jan 24, 2024
@EduPonz
Refs #20306: Add regression test
1e06775 
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
@EduPonz
Refs #20306: Ignore PID_NETWORK_CONFIGURATION_SET if coming from othe…
99723c2 
…r vendor

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
@EduPonz EduPonz force-pushed the hotfix/ignore_other_vendors_custom_pdp_pid branch from 2e97fdf to 99723c2 Compare January 24, 2024 15:12
@EduPonz
Copy link
Author

EduPonz commented Jan 24, 2024

@richiprosima please test this

@EduPonz EduPonz mentioned this pull request Jan 24, 2024
Merged
@EduPonz
Refs #20306: Apply Miguel's suggestions
b336b07 
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
@EduPonz
Copy link
Author

EduPonz commented Jan 24, 2024

@richiprosima please test this

@EduPonz
Refs #20306: Fix windows warning
27bced5 
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
@EduPonz EduPonz added the skip-ci Automatically pass CI label Jan 25, 2024
@EduPonz
Copy link
Author

EduPonz commented Jan 25, 2024

b336b07 had a windows warning on the test assertion due to signed vs unsigned comparison. This is fixed in 27bced5. I'm only running the build part of just the windows CI for 27bced5. To see the test results check b336b07 CI

@EduPonz EduPonz added no-test Skip CI tests if PR marked with this label and removed skip-ci Automatically pass CI labels Jan 25, 2024
@EduPonz
Copy link
Author

EduPonz commented Jan 25, 2024

@richiprosima please test windows

@MiguelCompany MiguelCompany added the ci-pending PR which CI is running label Jan 25, 2024
@MiguelCompany MiguelCompany merged commit 5a3acb4 into master Jan 25, 2024
10 checks passed
@MiguelCompany MiguelCompany deleted the hotfix/ignore_other_vendors_custom_pdp_pid branch January 25, 2024 11:29
@MiguelCompany MiguelCompany mentioned this pull request Jan 26, 2024
Merged
@JesusPoderoso JesusPoderoso added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed no-test Skip CI tests if PR marked with this label ci-pending PR which CI is running labels Jan 26, 2024
@EduPonz
Copy link
Author

EduPonz commented Jan 27, 2024

@Mergifyio backport 2.12.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 27, 2024 •
edited
Loading

backport 2.12.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 27, 2024
@EduPonz @mergify
Ignore 0x8007 if coming from other vendor (#4291)
71a0275 
* Refs #20306: Add regression test

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Ignore PID_NETWORK_CONFIGURATION_SET if coming from other vendor

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Apply Miguel's suggestions

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Fix windows warning

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

---------

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
(cherry picked from commit 5a3acb4)
@mergify mergify bot mentioned this pull request Jan 27, 2024
Merged
EduPonz added a commit that referenced this pull request Feb 16, 2024
@EduPonz
Ignore 0x8007 if coming from other vendor (#4291)
f60f783 
* Refs #20306: Add regression test

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Ignore PID_NETWORK_CONFIGURATION_SET if coming from other vendor

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Apply Miguel's suggestions

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Fix windows warning

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

---------

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
(cherry picked from commit 5a3acb4)
@MiguelCompany MiguelCompany mentioned this pull request Feb 19, 2024
Closed
1 task
EduPonz added a commit that referenced this pull request Feb 27, 2024
@mergify @EduPonz
Ignore 0x8007 if coming from other vendor (#4291) (#4309)
a00eb67 
* Ignore 0x8007 if coming from other vendor (#4291)

* Refs #20306: Add regression test

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Ignore PID_NETWORK_CONFIGURATION_SET if coming from other vendor

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Apply Miguel's suggestions

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20306: Fix windows warning

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

---------

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
(cherry picked from commit 5a3acb4)

* Fix compilation

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

---------

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Co-authored-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.13.2
Development

Successfully merging this pull request may close these issues.
3 participants
@EduPonz @MiguelCompany @JesusPoderoso