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

[jolavillette]

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

[csoler]

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.

[csoler]

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.

[csoler]

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

[jolavillette]

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::vector → std::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)

[csoler]

So in short, we cannot trust the AI.

[csoler]

If canonical.empty() is true, it means the path (including the link) is not valid. So there's no point is continuing here. You can also factor the two ifs, which gives:
if(!canonical.empty())
{
existing_dirs.insert(canonical);

std::set<std::string> current_branch_real_paths = {canonical};

recursUpdateSharedDir(
	            stored_dir_it.name(), *stored_dir_it,
	            existing_dirs, current_branch_real_paths, 1, some_files_not_ready );

}

[csoler]

Same here: if the path points to an ancestor, we should ignore it and return.

[csoler]

...so I suggest you replace all this by

  current_branch_real_paths.insert(canonical);
  recursUpdateSharedDirs(...);
  current_branch_real_path.erase(canonical);

and add a comment that canonical cannot be empty.