Skip to content
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

test if symlink already created before to avoid unnecessary exception #416

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test if symlink already created before to avoid unnecessary exception #416

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 18 additions & 3 deletions src/zimdump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,18 @@
write_to_file(directory + SEPARATOR, outputPath, content.c_str(), content.size());
}

static bool testSymlink(const std::string& sym, const std::string& target) {
char buf[256];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 256 ?
Symlink can be longer than 256, we should handle that.

Has we are C++17, we may simply use std::filesystem::read_link (https://en.cppreference.com/w/cpp/filesystem/read_symlink) (which may involve a more substantial change in our code base)

Copy link
Author

Choose a reason for hiding this comment

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

readlink() does not append a terminating null byte to buf. It will (silently) truncate the contents (to a length of bufsiz characters), in case the buffer is too small to hold all of the contents.

I read man page of readlink, so I want buffer to have one more byte than default 255(assuming hardcoded MAX_PATH) so that return value of readlink() can be safely placing trailing NULL character in buf.

I agree to use std::filesystem::read_link which solves truncation of longer path of readlink and also conditional compilation of Windows, I guess(Sorry, no win32 compiling experience for years.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I read man page of readlink, so I want buffer to have one more byte than default 255(assuming hardcoded MAX_PATH) so that return value of readlink() can be safely placing trailing NULL character in buf.

Yes, but assuming MAX_PATH is 256 is wrong. On my linux it is 4096. 256 is most of the time the max filename (so the size of one component in the path).
And even assuming 4096 is wrong, see https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html

The solution is to have a increasing buffer (vector<char> is a nice canditate) and increase the buffer until returned value of readlink() is smaller than the bufsize.
(Or let std::filesystem::read_link do this for us)

Copy link
Author

Choose a reason for hiding this comment

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

@mgautierfr Absolutely! I have created another PR to avoid these issues. Please kindly take a look. Thanks.

ssize_t n = readlink(sym.c_str(), buf, 255);

Check failure on line 296 in src/zimdump.cpp

View check run for this annotation

codefactor.io / CodeFactor

src/zimdump.cpp#L296 

Possible race condition (CWE-362, CWE-20) (flawfinder7-readlink)
if (n > 0){
buf[n] = '\0';
if (target == buf) {
return true;
}
}
return false;
}

void ZimDumper::dumpFiles(const std::string& directory, bool symlinkdump, std::function<bool (const char c)> nsfilter)
{
unsigned int truncatedFiles = 0;
Expand Down Expand Up @@ -340,9 +352,12 @@
auto blob = redirectItem.getData();
write_to_file(directory + SEPARATOR, relative_path, blob.data(), blob.size());
#else
if (symlink(redirectPath.c_str(), full_path.c_str()) != 0) {
throw std::runtime_error(
std::string("Error creating symlink from ") + full_path + " to " + redirectPath);
// There is a chance of symlink already created before. i.e. repeat run
if (!testSymlink(full_path, redirectPath)) {
if (symlink(redirectPath.c_str(), full_path.c_str()) != 0) {
throw std::runtime_error(
std::string("Error creating symlink from ") + full_path + " to " + redirectPath);
}
}
#endif
}
Expand Down