Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jolavillette wants to merge 2 commits into
base: master
Choose a base branch
from
+53 −15
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f25f08a
fix(file_sharing): prevent infinite loop on circular symlinks when du…
jolavillette Mar 18, 2026
d73b18b
fix(file_sharing): handle NULL from canonicalize_file_name and separa…
jolavillette Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 44 additions & 13 deletions src/file_sharing/directory_updater.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,15 @@ bool LocalDirectoryUpdater::sweepSharedDirectories(bool& some_files_not_ready)
{
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

@csoler csoler Mar 29, 2026

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(!canonical.empty())
existing_dirs.insert(canonical);
std::set<std::string> current_branch_real_paths;
Copy link
Copy Markdown
Contributor

@csoler csoler Apr 5, 2026

Choose a reason for hiding this comment

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

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 );

}

if(!canonical.empty())
current_branch_real_paths.insert(canonical);
recursUpdateSharedDir(
stored_dir_it.name(), *stored_dir_it,
existing_dirs, 1, some_files_not_ready );
existing_dirs, current_branch_real_paths, 1, some_files_not_ready );
/* here we need to use the list that was stored, instead of the shared
* dir list, because the two are not necessarily in the same order. */
}
Expand All @@ -240,7 +245,7 @@ bool LocalDirectoryUpdater::sweepSharedDirectories(bool& some_files_not_ready)

void LocalDirectoryUpdater::recursUpdateSharedDir(
const std::string& cumulated_path, DirectoryStorage::EntryIndex indx,
std::set<std::string>& existing_directories, uint32_t current_depth,
std::set<std::string>& existing_directories, std::set<std::string>& current_branch_real_paths, uint32_t current_depth,
bool& some_files_not_ready )
{
RS_DBG4("parsing directory \"", cumulated_path, "\" index: ", indx);
Expand Down Expand Up @@ -300,21 +305,37 @@ void LocalDirectoryUpdater::recursUpdateSharedDir(
|| (mMaxShareDepth == 0 && current_depth >= 64) )
dir_is_accepted = false;

if(dir_is_accepted && mFollowSymLinks && mIgnoreDuplicates)
if(dir_is_accepted && mFollowSymLinks)
{
std::string real_path = RsDirUtil::removeSymLinks(
RsDirUtil::makePath(cumulated_path, dirIt.file_name()) );

if( existing_directories.end() !=
existing_directories.find(real_path) )
if(real_path.empty())
{
RS_WARN( "Directory: \"", cumulated_path,
"\" has real path: \"", real_path,
"\" which already belongs to another "
"shared directory. Ignoring" );
RS_WARN( "Broken/circular symlink: \"", cumulated_path,
"/", dirIt.file_name(), "\". Ignoring." );
dir_is_accepted = false;
}
else existing_directories.insert(real_path);
else if(current_branch_real_paths.end() != current_branch_real_paths.find(real_path))
{
RS_WARN( "Circular symlink detected: \"", cumulated_path,
"\" points to ancestor \"", real_path,
"\". Ignoring." );
dir_is_accepted = false;
}
else if(mIgnoreDuplicates)
{
if( existing_directories.end() !=
existing_directories.find(real_path) )
{
RS_WARN( "Directory: \"", cumulated_path,
"\" has real path: \"", real_path,
"\" which already belongs to another "
"shared directory. Ignoring" );
dir_is_accepted = false;
}
else existing_directories.insert(real_path);
}
}

if(dir_is_accepted) subdirs.insert(dirIt.file_name());
Expand Down Expand Up @@ -364,9 +385,19 @@ void LocalDirectoryUpdater::recursUpdateSharedDir(
// go through the list of sub-dirs and recursively update
for( DirectoryStorage::DirIterator stored_dir_it(mSharedDirectories, indx);
stored_dir_it; ++stored_dir_it )
recursUpdateSharedDir( RsDirUtil::makePath(cumulated_path, stored_dir_it.name()),
*stored_dir_it, existing_directories,
{
std::string next_path = RsDirUtil::makePath(cumulated_path, stored_dir_it.name());
std::string canonical = RsDirUtil::removeSymLinks(next_path);
if(!canonical.empty())
current_branch_real_paths.insert(canonical);
Copy link
Copy Markdown
Contributor

@csoler csoler Apr 5, 2026

Choose a reason for hiding this comment

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

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


recursUpdateSharedDir( next_path,
*stored_dir_it, existing_directories, current_branch_real_paths,
current_depth+1, some_files_not_ready );

if(!canonical.empty())
Copy link
Copy Markdown
Contributor

@csoler csoler Apr 5, 2026

Choose a reason for hiding this comment

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

...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.

current_branch_real_paths.erase(canonical);
}
}

bool LocalDirectoryUpdater::filterFile(const std::string& fname) const
Expand Down
2 changes: 1 addition & 1 deletion src/file_sharing/directory_updater.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class LocalDirectoryUpdater: public HashStorageClient, public RsTickingThread
virtual void hash_callback(uint32_t client_param, const std::string& name, const RsFileHash& hash, uint64_t size);
virtual bool hash_confirm(uint32_t client_param) ;

void recursUpdateSharedDir(const std::string& cumulated_path, DirectoryStorage::EntryIndex indx, std::set<std::string>& existing_directories, uint32_t current_depth,bool& files_not_ready);
void recursUpdateSharedDir(const std::string& cumulated_path, DirectoryStorage::EntryIndex indx, std::set<std::string>& existing_directories, std::set<std::string>& current_branch_real_paths, uint32_t current_depth,bool& files_not_ready);
bool sweepSharedDirectories(bool &some_files_not_ready);

private:
Expand Down
9 changes: 8 additions & 1 deletion src/util/rsdir.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,8 +622,15 @@ std::string RsDirUtil::removeSymLinks(const std::string& path)
return path ;
#else
char *tmp = canonicalize_file_name(path.c_str()) ;
std::string result(tmp) ;

if(tmp == nullptr)
{
RS_WARN("removeSymLinks: cannot resolve \"", path,
"\". Broken or circular symlink?");
return std::string();
}

std::string result(tmp) ;
free(tmp);
return result ;
#endif
Expand Down
Loading