Skip to content

fix(file_sharing): prevent infinite loop on circular symlinks when duplicates are allowed#281

Open
jolavillette wants to merge 2 commits intoRetroShare:masterfrom
jolavillette:FixCircularSymlinks
Open

fix(file_sharing): prevent infinite loop on circular symlinks when duplicates are allowed#281
jolavillette wants to merge 2 commits intoRetroShare:masterfrom
jolavillette:FixCircularSymlinks

Conversation

@jolavillette
Copy link
Copy Markdown
Contributor

@jolavillette jolavillette commented Mar 18, 2026

fix(file_sharing): prevent infinite loop on circular symlinks when duplicates are allowed

@jolavillette jolavillette changed the title fix(file_sharing): prevent infinite loop on circular symlinks when dupicates are allowed fix(file_sharing): prevent infinite loop on circular symlinks when duplicates are allowed Mar 18, 2026
Copy link
Copy Markdown
Contributor

@csoler csoler left a comment

Choose a reason for hiding this comment

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

It seems to me that the AI has tried to re-do what we already do with existing_paths in a new structure (a std::vector, which is bad for find() btw), whereas the problem of circular paths is already handled. What may happen is that if there is a circular symlink, RsDirUtil::removeSymLinks will return an empty string, which is not accounter for. That's probably where the infinite loop comes from.

RS_DBG4("recursing into \"", stored_dir_it.name());

existing_dirs.insert(RsDirUtil::removeSymLinks(stored_dir_it.name()));
std::string canonical = RsDirUtil::removeSymLinks(stored_dir_it.name());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

when there is a loop in the sym links, canonicalize_file_name() will return nullptr and RsDirUtil::removeSymLinks() will return a null std::string (empty()=true). So inserting this empty string in existing_dirs has probably unwanted consequences.


if( existing_directories.end() !=
existing_directories.find(real_path) )
if (std::find(current_branch_real_paths.begin(), current_branch_real_paths.end(), real_path) != current_branch_real_paths.end())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bad idea to do a find on a std::vector. std::set would be wiser.

@jolavillette
Copy link
Copy Markdown
Contributor Author

AI says

Thanks for the review! Here's what this v2 addresses:

1. [removeSymLinks] NULL crash — fixed ✅

You were right: canonicalize_file_name() returns NULL on circular symlinks (ELOOP), and std::string(nullptr) is UB. Now guarded with a nullptr check, returning an empty string + RS_WARN.

2. std::vectorstd::set — fixed ✅

Agreed, std::find on a vector was a bad choice. Replaced with std::set and set::find().

3. Why current_branch_real_paths is still needed

The existing_directories set is only checked when mIgnoreDuplicates == true (old code: if(dir_is_accepted && mFollowSymLinks && mIgnoreDuplicates)). When the user disables "ignore duplicates", no cycle detection happens at all — the loop runs until depth 64, which is exponential (2^N paths with N symlinks).

current_branch_real_paths tracks only the ancestors on the current DFS branch, not all visited directories globally. This lets us:

  • Always detect cycles (regardless of mIgnoreDuplicates)
  • Still allow the same directory to appear in different branches when the user wants duplicates

The logic is now cleanly separated into 3 levels:

  1. real_path.empty() → broken/circular symlink (ELOOP from canonicalize_file_name)
  2. current_branch_real_paths.find() → symlink points to an ancestor (cycle)
  3. existing_directories.find() → duplicate across branches (only if mIgnoreDuplicates)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants