Implement a validity check for firstSN by squizz617 · Pull Request #3274 · eProsima/Fast-DDS

squizz617 · 2023-02-07T00:57:30Z

This commit suggests a fix for issue #3236.

Description

As per 8.3.8.6.3 of DDS-RTPS 2.5, "Heartbeat submessage is invalid if firstSN.value is zero or negative". However, a check for this property is missing. This leads to an assertion failure when an invalid firstSN is provided (see issue #3236).

@Mergifyio backport 2.9.x

Fixes #3236

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) Changes are ABI compatible.
(N/A) Changes are API compatible.
(N/A) Documentation builds and tests pass locally.
(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

Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

Following 8.3.8.6.3 of DDS-RTPS 2.5. This fixes issue eProsima#3236. Signed-off-by: Seulbae Kim <squizz617@gmail.com>

MiguelCompany

@squizz617 Thank you for your contribution!

This is not building (see my comment below).

src/cpp/rtps/messages/MessageReceiver.cpp

Signed-off-by: Seulbae Kim <squizz617@gmail.com>

squizz617 · 2023-02-07T14:08:45Z

Sorry, made a mistake while moving stuffs into the pr branch :(
Fixed and pushed!

squizz617 · 2023-03-15T02:31:39Z

Hi @MiguelCompany, I'm just wondering if you've looked at the changes. Thank you.

MiguelCompany · 2023-03-15T07:48:29Z

Hi @MiguelCompany, I'm just wondering if you've looked at the changes. Thank you.

@squizz617 Changes look good to me. Is it possible that you could add a regression test for this?

I think that adding a new file here would be enough

Signed-off-by: Seulbae Kim <squizz617@gmail.com>

squizz617 · 2023-03-15T14:00:08Z

@MiguelCompany Done! I temporarily named the file input_issue3236, but please change it if needed.

MiguelCompany · 2023-03-15T14:49:52Z

@richiprosima Please test this

MiguelCompany · 2023-03-16T15:36:03Z

@Mergifyio backport 2.9.x 2.8.x 2.6.x

mergify · 2023-03-16T15:36:07Z

backport 2.9.x 2.8.x 2.6.x

✅ Backports have been created

Details

#3382 Implement a validity check for firstSN (backport #3274) has been created for branch 2.9.x
#3383 Implement a validity check for firstSN (backport #3274) has been created for branch 2.8.x
#3384 Implement a validity check for firstSN (backport #3274) has been created for branch 2.6.x

* Implement a validity check for firstSN Following 8.3.8.6.3 of DDS-RTPS 2.5. This fixes issue #3236. Signed-off-by: Seulbae Kim <squizz617@gmail.com> * fix typo Signed-off-by: Seulbae Kim <squizz617@gmail.com> * add test input for issue #3236 (pr #3274) Signed-off-by: Seulbae Kim <squizz617@gmail.com> --------- Signed-off-by: Seulbae Kim <squizz617@gmail.com> (cherry picked from commit 3aa3ee0)

* Implement a validity check for firstSN Following 8.3.8.6.3 of DDS-RTPS 2.5. This fixes issue #3236. Signed-off-by: Seulbae Kim <squizz617@gmail.com> * fix typo Signed-off-by: Seulbae Kim <squizz617@gmail.com> * add test input for issue #3236 (pr #3274) Signed-off-by: Seulbae Kim <squizz617@gmail.com> --------- Signed-off-by: Seulbae Kim <squizz617@gmail.com> (cherry picked from commit 3aa3ee0) Co-authored-by: Seulbae Kim <squizz617@gmail.com>

* Implement a validity check for firstSN (#3274) * Implement a validity check for firstSN Following 8.3.8.6.3 of DDS-RTPS 2.5. This fixes issue #3236. Signed-off-by: Seulbae Kim <squizz617@gmail.com> * fix typo Signed-off-by: Seulbae Kim <squizz617@gmail.com> * add test input for issue #3236 (pr #3274) Signed-off-by: Seulbae Kim <squizz617@gmail.com> --------- Signed-off-by: Seulbae Kim <squizz617@gmail.com> (cherry picked from commit 3aa3ee0) * Refs #17717: Logging Macro fix Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> --------- Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Seulbae Kim <squizz617@gmail.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com>

Implement a validity check for firstSN

60c15ed

Following 8.3.8.6.3 of DDS-RTPS 2.5. This fixes issue eProsima#3236. Signed-off-by: Seulbae Kim <squizz617@gmail.com>

MiguelCompany requested changes Feb 7, 2023

View reviewed changes

src/cpp/rtps/messages/MessageReceiver.cpp Outdated Show resolved Hide resolved

fix typo

4382b5d

Signed-off-by: Seulbae Kim <squizz617@gmail.com>

squizz617 force-pushed the pr-fix-3236 branch from 5b49eee to 4382b5d Compare February 7, 2023 14:06

MiguelCompany added this to the v2.10.0 milestone Mar 15, 2023

add test input for issue eProsima#3236 (pr eProsima#3274)

ffac167

Signed-off-by: Seulbae Kim <squizz617@gmail.com>

MiguelCompany approved these changes Mar 15, 2023

View reviewed changes

MiguelCompany added the ci-pending PR which CI is running label Mar 15, 2023

MRicoIE2CS added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Mar 16, 2023

MiguelCompany merged commit 3aa3ee0 into eProsima:master Mar 16, 2023

mergify bot mentioned this pull request Mar 16, 2023

Implement a validity check for firstSN (backport #3274) #3382

Merged

mergify bot mentioned this pull request Mar 16, 2023

Implement a validity check for firstSN (backport #3274) #3383

Merged

mergify bot mentioned this pull request Mar 16, 2023

Implement a validity check for firstSN (backport #3274) #3384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a validity check for firstSN#3274

Implement a validity check for firstSN#3274
MiguelCompany merged 3 commits intoeProsima:masterfrom
squizz617:pr-fix-3236

squizz617 commented Feb 7, 2023 •

edited by MRicoIE2CS

Loading

Uh oh!

MiguelCompany left a comment

Uh oh!

Uh oh!

squizz617 commented Feb 7, 2023

Uh oh!

squizz617 commented Mar 15, 2023

Uh oh!

MiguelCompany commented Mar 15, 2023

Uh oh!

squizz617 commented Mar 15, 2023 •

edited

Loading

Uh oh!

MiguelCompany commented Mar 15, 2023

Uh oh!

MiguelCompany commented Mar 16, 2023

Uh oh!

mergify bot commented Mar 16, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

squizz617 commented Feb 7, 2023 • edited by MRicoIE2CS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Checklist

Reviewer Checklist

Uh oh!

MiguelCompany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

squizz617 commented Feb 7, 2023

Uh oh!

squizz617 commented Mar 15, 2023

Uh oh!

MiguelCompany commented Mar 15, 2023

Uh oh!

squizz617 commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiguelCompany commented Mar 15, 2023

Uh oh!

MiguelCompany commented Mar 16, 2023

Uh oh!

mergify bot commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

squizz617 commented Feb 7, 2023 •

edited by MRicoIE2CS

Loading

squizz617 commented Mar 15, 2023 •

edited

Loading

mergify bot commented Mar 16, 2023 •

edited

Loading