Fix cleanup race condition with exclusive and shared lock files #5319

ma30002000 · 2024-10-14T12:32:58Z

Description

Since on unix file opening and locking are non-atomic operations (open() system call followed with a flock() call), and clean.py depends on exclusive lockability to in order to decide on zombie file deletion, the following race condition (although rare) exists:

RobustExclusiveLock.hpp RobustExclusiveLock::open_and_lock_file calls open() successfully
clean.py opens, successfully locks and deletes the file
RobustExclusiveLock.hpp RobustExclusiveLock::open_and_lock_file calls flock() successfully

This leads to the file's creator assuming a successful file creation (the fd is valid and usable), while in reality the file is not accessible anymore since its directory entry has been removed. A similar situation might be triggered in obustSharedLock.hpp.

Verifying the existence of the directory entry with a call to stat() after successfully obtaining the exclusive lock of the file fixes and restarting the open and lock procedure in case the file was removed in between seems to fix the behavior.

@Mergifyio backport 3.1.x 2.14.x 2.10.x

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
N/A 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.
N/A 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.

…lusiveLock Signed-off-by: Matthias Schneider <[email protected]>

…redLock Signed-off-by: Matthias Schneider <[email protected]>

ma30002000 · 2024-10-14T12:41:26Z

Note that this PR does NOT address #21861, which deals with files missing from the cleanup procedure.

ma30002000 · 2024-10-14T12:59:43Z

I have removed the references to #5308 since this PR deals with a different issue.

src/cpp/utils/shared_memory/RobustExclusiveLock.hpp

src/cpp/utils/shared_memory/RobustSharedLock.hpp

Signed-off-by: Matthias Schneider <[email protected]>

MiguelCompany

LGTM with green CI

MiguelCompany · 2025-03-07T12:11:35Z

Mac failure seems unrelated, going in.
Thank you @ma30002000 !

MiguelCompany · 2025-03-07T12:12:06Z

@Mergifyio backport 3.1.x 2.14.x 2.10.x

mergify · 2025-03-07T12:12:15Z

backport 3.1.x 2.14.x 2.10.x

✅ Backports have been created

#5693 Fix cleanup race condition with exclusive and shared lock files (backport #5319) has been created for branch 3.1.x
#5694 Fix cleanup race condition with exclusive and shared lock files (backport #5319) has been created for branch 2.14.x
#5695 Fix cleanup race condition with exclusive and shared lock files (backport #5319) has been created for branch 2.10.x

* Prevent race condition with concurrent cleanup operation in RobustExclusiveLock Signed-off-by: Matthias Schneider <[email protected]> * Prevent race condition with concurrent cleanup operation in RobustSharedLock Signed-off-by: Matthias Schneider <[email protected]> * Adapted coding style as suggested by MiguelCompany Signed-off-by: Matthias Schneider <[email protected]> --------- Signed-off-by: Matthias Schneider <[email protected]> (cherry picked from commit 53bf6ab)

… (#5693) * Prevent race condition with concurrent cleanup operation in RobustExclusiveLock Signed-off-by: Matthias Schneider <[email protected]> * Prevent race condition with concurrent cleanup operation in RobustSharedLock Signed-off-by: Matthias Schneider <[email protected]> * Adapted coding style as suggested by MiguelCompany Signed-off-by: Matthias Schneider <[email protected]> --------- Signed-off-by: Matthias Schneider <[email protected]> (cherry picked from commit 53bf6ab) Co-authored-by: Matthias Schneider <[email protected]>

… (#5694) * Prevent race condition with concurrent cleanup operation in RobustExclusiveLock Signed-off-by: Matthias Schneider <[email protected]> * Prevent race condition with concurrent cleanup operation in RobustSharedLock Signed-off-by: Matthias Schneider <[email protected]> * Adapted coding style as suggested by MiguelCompany Signed-off-by: Matthias Schneider <[email protected]> --------- Signed-off-by: Matthias Schneider <[email protected]> (cherry picked from commit 53bf6ab) Co-authored-by: Matthias Schneider <[email protected]>

* Prevent race condition with concurrent cleanup operation in RobustExclusiveLock Signed-off-by: Matthias Schneider <[email protected]> * Prevent race condition with concurrent cleanup operation in RobustSharedLock Signed-off-by: Matthias Schneider <[email protected]> * Adapted coding style as suggested by MiguelCompany Signed-off-by: Matthias Schneider <[email protected]> --------- Signed-off-by: Matthias Schneider <[email protected]> (cherry picked from commit 53bf6ab)

ma30002000 added 2 commits October 14, 2024 14:27

Prevent race condition with concurrent cleanup operation in RobustExc…

d4bbcdb

…lusiveLock Signed-off-by: Matthias Schneider <[email protected]>

Prevent race condition with concurrent cleanup operation in RobustSha…

109deba

…redLock Signed-off-by: Matthias Schneider <[email protected]>

ma30002000 force-pushed the bugfix/fix-open-lock-racecondition branch from 850df66 to 109deba Compare October 14, 2024 12:38

JesusPoderoso added this to the v3.1.1 milestone Oct 14, 2024

JesusPoderoso changed the title ~~Fix cleanup race condition with exclusive and shareed lock files~~ [21861] Fix cleanup race condition with exclusive and shared lock files Oct 14, 2024

JesusPoderoso added the needs-review PR that is ready to be reviewed label Oct 14, 2024

ma30002000 changed the title ~~[21861] Fix cleanup race condition with exclusive and shared lock files~~ Fix cleanup race condition with exclusive and shared lock files Oct 14, 2024

JesusPoderoso modified the milestones: v3.1.1, v3.2.0 Oct 17, 2024

MiguelCompany requested changes Mar 5, 2025

View reviewed changes

src/cpp/utils/shared_memory/RobustExclusiveLock.hpp Outdated Show resolved Hide resolved

src/cpp/utils/shared_memory/RobustSharedLock.hpp Outdated Show resolved Hide resolved

MiguelCompany removed the needs-review PR that is ready to be reviewed label Mar 5, 2025

Adapted coding style as suggested by MiguelCompany

d1271b6

Signed-off-by: Matthias Schneider <[email protected]>

MiguelCompany self-requested a review March 7, 2025 06:47

MiguelCompany added the ci-pending PR which CI is running label Mar 7, 2025

MiguelCompany approved these changes Mar 7, 2025

View reviewed changes

MiguelCompany merged commit 53bf6ab into eProsima:master Mar 7, 2025
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cleanup race condition with exclusive and shared lock files #5319

Fix cleanup race condition with exclusive and shared lock files #5319

ma30002000 commented Oct 14, 2024 •

edited by MiguelCompany

Loading

ma30002000 commented Oct 14, 2024

ma30002000 commented Oct 14, 2024 •

edited

Loading

MiguelCompany left a comment

MiguelCompany commented Mar 7, 2025

MiguelCompany commented Mar 7, 2025

mergify bot commented Mar 7, 2025 •

edited

Loading

Fix cleanup race condition with exclusive and shared lock files #5319

Fix cleanup race condition with exclusive and shared lock files #5319

Conversation

ma30002000 commented Oct 14, 2024 • edited by MiguelCompany Loading

Description

Contributor Checklist

Reviewer Checklist

ma30002000 commented Oct 14, 2024

ma30002000 commented Oct 14, 2024 • edited Loading

MiguelCompany left a comment

Choose a reason for hiding this comment

MiguelCompany commented Mar 7, 2025

MiguelCompany commented Mar 7, 2025

mergify bot commented Mar 7, 2025 • edited Loading

✅ Backports have been created

ma30002000 commented Oct 14, 2024 •

edited by MiguelCompany

Loading

ma30002000 commented Oct 14, 2024 •

edited

Loading

mergify bot commented Mar 7, 2025 •

edited

Loading