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

[21362] Fail when trying to serialize std::string with null characters on its content #245

Merged
merged 6 commits into from
Nov 8, 2024
Merged

[21362] Fail when trying to serialize std::string with null characters on its content #245

merged 6 commits into from
Nov 8, 2024

Conversation

MiguelCompany
Copy link
Member

@MiguelCompany MiguelCompany commented Nov 4, 2024 •
edited by Mario-DL
Loading

Description

This addresses #69 and #214 by explicitly failing to serialize a string that contains null characters, instead of silently discard the last part of the string.

@Mergifyio backport 2.1.x 1.0.x
Note: I'm not sure whether we should backport this to 1.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
  • Any new/modified methods have been properly documented using Doxygen.
  • 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).
  • 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: CI pass and failing tests are unrelated with the changes.

@MiguelCompany MiguelCompany requested review from richiprosima and removed request for richiprosima November 4, 2024 16:01
@MiguelCompany
Refs #21362. Add test.
5f961ad 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Refs #21362. Fix Cdr behavior.
ecc6cf8 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Refs #21362. Fix FastCDR behavior.
5a109ef 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany MiguelCompany requested review from richiprosima and removed request for richiprosima November 5, 2024 11:20
@MiguelCompany
Refs #21362. Fix dll export.
3debdec 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany MiguelCompany requested review from richiprosima and removed request for richiprosima November 5, 2024 12:05
@MiguelCompany MiguelCompany changed the title [21362] Fail when trying to serialize std::string with null characters on its content [22071] Fail when trying to serialize std::string with null characters on its content Nov 6, 2024
@MiguelCompany
Refs #21362. Leave implementation in header.
0790a73 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany MiguelCompany added this to the v2.2.6 milestone Nov 6, 2024
@MiguelCompany MiguelCompany marked this pull request as ready for review November 6, 2024 11:00
@MiguelCompany MiguelCompany requested review from richiprosima and removed request for richiprosima November 6, 2024 11:00
@MiguelCompany MiguelCompany changed the title [22071] Fail when trying to serialize std::string with null characters on its content [21362] Fail when trying to serialize std::string with null characters on its content Nov 6, 2024
@MiguelCompany MiguelCompany added needs-review Mark this PR as ready to be reviewed and removed ci-pending labels Nov 6, 2024
Mario-DL
Mario-DL requested changes Nov 8, 2024
View reviewed changes
src/cpp/Cdr.cpp Outdated Show resolved Hide resolved
@Mario-DL
Copy link
Member

Mario-DL commented Nov 8, 2024

Should be also make the changes when serializing a wstring ?

@MiguelCompany @Mario-DL
Refs #21362. Apply suggestion.
b0b5fac 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>
@MiguelCompany
Copy link
Member Author

Should be also make the changes when serializing a wstring ?

Not sure, the standard says that for strings, the terminating null character is not transmitted. Not sure if that implies that the string can have null characters in the middle. I'd say we can handle that on a different PR

@MiguelCompany MiguelCompany requested review from Mario-DL and removed request for richiprosima November 8, 2024 08:51
Mario-DL
Mario-DL approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

LGTM

@MiguelCompany
Copy link
Member Author

@Mergifyio backport 2.1.x

@mergify Mergify
Copy link

mergify bot commented Nov 8, 2024 •
edited
Loading

backport 2.1.x

✅ Backports have been created

@MiguelCompany MiguelCompany merged commit 5cc8c55 into master Nov 8, 2024
11 checks passed
@MiguelCompany MiguelCompany deleted the hotfix/21362 branch November 8, 2024 09:06
mergify bot pushed a commit that referenced this pull request Nov 8, 2024
@MiguelCompany @mergify
Fail when trying to serialize std::string with null characters on i…
33277e7 
…ts content (#245)

* Refs #21362. Add test.

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

* Refs #21362. Fix Cdr behavior.

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

* Refs #21362. Fix FastCDR behavior.

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

* Refs #21362. Fix dll export.

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

* Refs #21362. Leave implementation in header.

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

* Refs #21362. Apply suggestion.

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

Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>

---------

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>
(cherry picked from commit 5cc8c55)
@mergify mergify bot mentioned this pull request Nov 8, 2024
Closed
12 tasks
@MiguelCompany MiguelCompany mentioned this pull request Nov 26, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mario-DL Mario-DL Mario-DL approved these changes

Assignees
No one assigned
Labels
ci-pending needs-review Mark this PR as ready to be reviewed
Projects
None yet
Milestone
v2.2.6
Development

Successfully merging this pull request may close these issues.
2 participants
@MiguelCompany @Mario-DL