test if symlink already created before to avoid unnecessary exception #416
Conversation
|
I am not very sure about this "possible race condition" from CodeFactor (Possible race condition (CWE-362, CWE-20) (flawfinder7-readlink)). My guess is that this is only an improvement of one case when symlink already created. Say there is a race condition that a symlink is created after test is done so that we end up as the same exception. But this is NO WORSE than current code. So, I see no harm of this extra test to reduce some exception-throwing. It is NOT perfect solution, but just an improvement. |
There was a problem hiding this comment.
I am not very sure about this "possible race condition" from CodeFactor [...]
Indeed, the attack vector seems a bit small here.
At best, we have a way to the attacker to get zimdump not generate a link or throw the exception. But if attacker has access to file system, it would better have to remove/rewrite link after dump.
However, as a better design, it would be better to blindly try to create the symlink and check existing symlink only if creation fails.
This way we avoid extra system call which will fail most of the time (existing symlink is a exceptional case)
| @@ -291,6 +291,18 @@ void ZimDumper::writeHttpRedirect(const std::string& directory, const std::strin | |||
| 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]; | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@mgautierfr Absolutely! I have created another PR to avoid these issues. Please kindly take a look. Thanks.
In case zimdump run repeatedly, the symlink could be created already. So, before throwing exception, better to call "readLink" to check symlink.