Fix volatile reader desync after initial positioning GAP#6456
Open
cferreiragonz wants to merge 1 commit into
Open
Fix volatile reader desync after initial positioning GAP#6456cferreiragonz wants to merge 1 commit into
cferreiragonz wants to merge 1 commit into
Conversation
* Refs #24631: Regression Test Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Limit GAP processing to HB announced value Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Send a HB before every GAP Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Update tests Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Fix writer's assumption of reader state after GAP sent Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Uncrustify Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Update versions.md Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Skip GAPs for changes removed from history Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Fix Datasharing GAP reception Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Handle sample processed as lost instead of GAP'ed Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Send HB in process_acknack iff GAP is required Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #24631: Coverage test for unknown sequence number comparison Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> --------- Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is an alternative to fix proposed for CVE-2025-64438 that limited the maximum number of GAPs that a reader could process. This fix had a problem with volatile late-joiners readers that caused a de-synchronization between reader and writer, resulting in the reader not processing any message. The key is that the writer assumes the reader's state after sending the first initial positioning GAP in
StatefulWriter::matched_reader_add_edp. Hence, later ACKs requesting old data are simply ignored and the reader never syncs with the writer, preventing real communication even though DATA is correctly being received at the transport layer.The issue comes, thus, from two different conditions:
The new fix forces sending a HB before every GAP and limiting the GAP processing to the value specified in the heartbeat. It also allows the Writer to send GAP messages if a later ACK is received requesting old samples.
@Mergifyio backport 3.2.x 2.14.x
Contributor Checklist
versions.mdfile (if applicable).Reviewer Checklist