[21751] Discard changes with big key-only payload and no key hash #5262

MiguelCompany · 2024-09-25T08:47:38Z

Description

We currently don't have support for key-only payloads bigger than the KEY_HASH length.
Receiving such kind of messages produces a segfault in DataReaderHistory.

This PR adds a regression test reproducing the issue, and fixes it by considering the incoming change as irrelevant.
It has been done this way so it is acknowledged in reliable topics, thus avoiding the offending data to be resent.

@Mergifyio backport 2.14.x 2.10.x

@Mergifyio backport 2.6.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.
N/A: Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
Changes are backport compatible: they do NOT break ABI nor change library core behavior.
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.
If this is a critical bug fix, backports to the critical-only supported branches have been requested.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

…y payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

test/blackbox/common/BlackboxTestsTransportUDP.cpp

src/cpp/rtps/reader/reader_utils.cpp

src/cpp/rtps/messages/MessageReceiver.cpp

…ter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany · 2024-09-27T08:16:12Z

@Mergifyio backport 2.14.x

mergify · 2024-09-27T08:16:17Z

backport 2.14.x

✅ Backports have been created

#5274 [21751] Discard changes with big key-only payload and no key hash (backport #5262) has been created for branch 2.14.x

* Refs #21707. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document else. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 175add5)

MiguelCompany · 2024-09-27T08:24:04Z

@Mergifyio backport 2.10.x 2.6.x

mergify · 2024-09-27T08:24:11Z

backport 2.10.x 2.6.x

✅ Backports have been created

#5275 [21751] Discard changes with big key-only payload and no key hash (backport #5262) has been created for branch 2.10.x but encountered conflicts
#5276 [21751] Discard changes with big key-only payload and no key hash (backport #5262) has been created for branch 2.6.x but encountered conflicts

* Refs #21707. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document else. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 175add5) # Conflicts: # test/blackbox/common/BlackboxTestsTransportUDP.cpp

* Refs #21707. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document else. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 175add5) # Conflicts: # src/cpp/rtps/messages/MessageReceiver.cpp # test/blackbox/common/BlackboxTestsTransportUDP.cpp

) * Discard changes with big key-only payload and no key hash (#5262) * Refs #21707. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document else. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 175add5) * Fix build Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>

) * Discard changes with big key-only payload and no key hash (#5262) * Refs #21707. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document else. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 175add5) # Conflicts: # src/cpp/rtps/messages/MessageReceiver.cpp # test/blackbox/common/BlackboxTestsTransportUDP.cpp * Fix conflicts Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Additional fixes Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> --------- Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com>

) * Discard changes with big key-only payload and no key hash (#5262) * Refs #21707. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Document else. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 175add5) # Conflicts: # test/blackbox/common/BlackboxTestsTransportUDP.cpp * Fix conflicts Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Fix build Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>

Refs #21707. Regression test.

ae9ca37

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany added this to the v3.0.2 milestone Sep 25, 2024

MiguelCompany requested a review from richiprosima September 25, 2024 08:53

github-actions bot added the ci-pending PR which CI is running label Sep 25, 2024

MiguelCompany added 5 commits September 25, 2024 10:55

Refs #21707. Test both best_effort and reliable.

2ea6b94

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Refs #21707. Fix by considering offending change as irrelevant.

12821f7

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Refs #21707. Refactor code in MessageReceiver.

ffccf15

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Refs #21707. Avoid overriding instanceHandle.

2e5ffb0

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Refs #21707. Add TODO so we don't forget to correctly process key-onl…

73f3586

…y payloads in the future. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany force-pushed the bugfix/21707 branch from 3a035d5 to 73f3586 Compare September 25, 2024 08:55

MiguelCompany requested review from richiprosima and removed request for richiprosima September 25, 2024 08:55

richiware requested changes Sep 26, 2024

View reviewed changes

MiguelCompany added 3 commits September 26, 2024 08:05

Refs #21751. Document and improve code in `change_is_relevant_for_fil…

43ae9a6

…ter`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Refs #21751. Document else.

750eaa0

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Refs #21751. Added static assertions for structure sizes.

125ea60

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany requested a review from richiware September 26, 2024 07:48

richiware approved these changes Sep 27, 2024

View reviewed changes

MiguelCompany merged commit 175add5 into master Sep 27, 2024
17 checks passed

MiguelCompany deleted the bugfix/21707 branch September 27, 2024 08:16

mergify bot mentioned this pull request Sep 27, 2024

[21751] Discard changes with big key-only payload and no key hash (backport #5262) #5274

Merged

12 tasks

mergify bot mentioned this pull request Sep 27, 2024

[21751] Discard changes with big key-only payload and no key hash (backport #5262) #5275

Merged

12 tasks

mergify bot mentioned this pull request Sep 27, 2024

[21751] Discard changes with big key-only payload and no key hash (backport #5262) #5276

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[21751] Discard changes with big key-only payload and no key hash #5262

[21751] Discard changes with big key-only payload and no key hash #5262

MiguelCompany commented Sep 25, 2024 •

edited by richiware

Loading

MiguelCompany commented Sep 27, 2024

mergify bot commented Sep 27, 2024 •

edited

Loading

MiguelCompany commented Sep 27, 2024

mergify bot commented Sep 27, 2024 •

edited

Loading

[21751] Discard changes with big key-only payload and no key hash #5262

[21751] Discard changes with big key-only payload and no key hash #5262

Conversation

MiguelCompany commented Sep 25, 2024 • edited by richiware Loading

Description

Contributor Checklist

Reviewer Checklist

MiguelCompany commented Sep 27, 2024

mergify bot commented Sep 27, 2024 • edited Loading

✅ Backports have been created

MiguelCompany commented Sep 27, 2024

mergify bot commented Sep 27, 2024 • edited Loading

✅ Backports have been created

MiguelCompany commented Sep 25, 2024 •

edited by richiware

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024 •

edited

Loading