Skip to content

Fix deserialization of maps into non-empty maps - #340

Merged
MiguelCompany merged 3 commits into
eProsima:masterfrom
shashenli:fix-339-map-deserialization
Sep 7, 2026
Merged

Fix deserialization of maps into non-empty maps#340
MiguelCompany merged 3 commits into
eProsima:masterfrom
shashenli:fix-339-map-deserialization

Conversation

@shashenli

@shashenli shashenli commented Sep 7, 2026 •
edited by MiguelCompany
Loading

Copy link
Copy Markdown
Contributor

Description

As reported in #339, Cdr::deserialize(std::map<K, T>&) presents two problems when the output map is not empty:

  • The overload for primitive (arithmetic / enum) mapped types never cleared the output map, so deserializing into an already used map (e.g. a sample reused by DataReader::take_next_sample()) silently kept stale keys and values.
  • Both overloads modified the output map (cleared it and/or added elements) before completing the deserialization, so a NotEnoughMemoryException thrown inside the loop left the output map cleared or partially filled.

Following the suggestion in #339, both overloads now deserialize the elements into a new map, which is moved into the output one once the whole deserialization succeeds. This way the previous content of the output map is always replaced, and it is preserved if the deserialization fails.

Two commits:

  • First commit adds tests for correct/incorrect deserialization of primitive/non-primitive maps, including the case of reusing the same map for two deserialization calls. The tests fail on current master.
  • Second commit contains the fix. All tests pass locally (2118/2118 ctest).

@Mergifyio backport 2.3.x

Fixes #339

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • 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). Bug fix, no new feature.
  • Applicable backports have been included in the description. 2.3.x is affected (reported against 2.3.6).

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.

@shashenli
Add tests for deserialization of maps into non-empty maps
7a085b2 
Tests added for both primitive and non-primitive mapped types:
- Deserialization into a non-empty map.
- Reuse of the same target map in two consecutive deserializations.
- Content of the target map preserved when the deserialization fails.

These tests show the issues described in eProsima#339.

Signed-off-by: lijiapei <l2249712344@gmail.com>
@shashenli
Fix deserialization of maps into non-empty maps
e3d2d73 
Deserialize into a new map and move it into the output one once the
whole deserialization succeeds.

This way the output map is not left cleared or partially filled when an
exception is thrown inside the deserialization loop, and the previous
content of a non-empty output map is always replaced. This applies to
both the primitive and non-primitive mapped type overloads.

Fixes eProsima#339.

Signed-off-by: lijiapei <l2249712344@gmail.com>
@shashenli

shashenli commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@MiguelCompany

@MiguelCompany MiguelCompany added this to the v2.4.1 milestone Sep 7, 2026
@MiguelCompany
MiguelCompany self-requested a review September 7, 2026 06:59
@shashenli
Fix MSVC C4244 warning in map deserialization tests
97b119c 
Use explicit static_cast<uint16_t> for map keys, as done in
DeserializeIntoANonEmptyMapInXCDRv1. int literals converted to
uint16_t trigger warning C4244, treated as error by Windows CI.

Signed-off-by: lijiapei <l2249712344@gmail.com>
@shashenli
shashenli force-pushed the fix-339-map-deserialization branch from 03fe9f6 to 97b119c Compare September 7, 2026 07:46
@shashenli

shashenli commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Hi @MiguelCompany,

The Windows CI failure was caused by the tests added in this PR: MSVC raises warning C4244 (int → uint16_t narrowing in the std::map<uint16_t, std::string> initializers), which the Windows CI treats as an error. Ubuntu and macOS are not affected.

Fixed in 97b119c by using static_cast<uint16_t> for the map keys, following the same pattern already used in DeserializeIntoANonEmptyMapInXCDRv1.

Since the CI workflows only trigger on review_requested, could you re-trigger them (or re-request the review) for the new commit? The CodeQL run is also waiting for approval. Thanks!

@MiguelCompany
MiguelCompany requested review from MiguelCompany and removed request for MiguelCompany September 7, 2026 08:10
@MiguelCompany

MiguelCompany commented Sep 7, 2026

Copy link
Copy Markdown
Member

@shashenli Thank you for your contribution!

@MiguelCompany
MiguelCompany merged commit b2457c4 into eProsima:master Sep 7, 2026
16 checks passed
@MiguelCompany

MiguelCompany commented Sep 7, 2026

Copy link
Copy Markdown
Member

@Mergifyio backport 2.3.x 2.2.x

@mergify

mergify Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown

backport 2.3.x 2.2.x

✅ Backports have been created

Details

This was referenced Sep 7, 2026
Merged
Merged
MiguelCompany pushed a commit that referenced this pull request Sep 7, 2026
@mergify @shashenli
Fix deserialization of maps into non-empty maps (#340) (#342)
757d5e4 
* Add tests for deserialization of maps into non-empty maps

Tests added for both primitive and non-primitive mapped types:
- Deserialization into a non-empty map.
- Reuse of the same target map in two consecutive deserializations.
- Content of the target map preserved when the deserialization fails.

These tests show the issues described in #339.



* Fix deserialization of maps into non-empty maps

Deserialize into a new map and move it into the output one once the
whole deserialization succeeds.

This way the output map is not left cleared or partially filled when an
exception is thrown inside the deserialization loop, and the previous
content of a non-empty output map is always replaced. This applies to
both the primitive and non-primitive mapped type overloads.

Fixes #339.



* Fix MSVC C4244 warning in map deserialization tests

Use explicit static_cast<uint16_t> for map keys, as done in
DeserializeIntoANonEmptyMapInXCDRv1. int literals converted to
uint16_t trigger warning C4244, treated as error by Windows CI.



---------


(cherry picked from commit b2457c4)

Signed-off-by: lijiapei <l2249712344@gmail.com>
Co-authored-by: lijiapei <lilingjie@cvte.com>
MiguelCompany pushed a commit that referenced this pull request Sep 7, 2026
@mergify @shashenli
Fix deserialization of maps into non-empty maps (#340) (#341)
b2a1738 
* Add tests for deserialization of maps into non-empty maps

Tests added for both primitive and non-primitive mapped types:
- Deserialization into a non-empty map.
- Reuse of the same target map in two consecutive deserializations.
- Content of the target map preserved when the deserialization fails.

These tests show the issues described in #339.



* Fix deserialization of maps into non-empty maps

Deserialize into a new map and move it into the output one once the
whole deserialization succeeds.

This way the output map is not left cleared or partially filled when an
exception is thrown inside the deserialization loop, and the previous
content of a non-empty output map is always replaced. This applies to
both the primitive and non-primitive mapped type overloads.

Fixes #339.



* Fix MSVC C4244 warning in map deserialization tests

Use explicit static_cast<uint16_t> for map keys, as done in
DeserializeIntoANonEmptyMapInXCDRv1. int literals converted to
uint16_t trigger warning C4244, treated as error by Windows CI.



---------


(cherry picked from commit b2457c4)

Signed-off-by: lijiapei <l2249712344@gmail.com>
Co-authored-by: lijiapei <lilingjie@cvte.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

None yet

Projects

None yet

Milestone

v2.4.1

Development

Successfully merging this pull request may close these issues.

Cdr::deserialize(std::map<K, T>&) does not clear() the map when T is arithmetic/enum — stale values survive deserializing into a reused object

2 participants

@shashenli @MiguelCompany