Repository navigation
Make ancestor directories before writing htoprc - #1850
Explorer09 wants to merge 3 commits into
Conversation
| int ret = 0; | ||
| int savedErr = errno; | ||
|
|
||
| char *p = filePath + len; | ||
| for (; p > filePath; p--) { | ||
| if (*p != '/') | ||
| continue; | ||
|
|
||
| *p = '\0'; | ||
| bool ok = !mkdir(filePath, S_IRWXU); | ||
| *p = '/'; | ||
| if (ok) | ||
| break; | ||
|
|
||
| if (errno != ENOENT) | ||
| return -errno; | ||
|
|
||
| ret = -errno; | ||
| } | ||
| if (p <= filePath) | ||
| return ret; | ||
|
|
||
| errno = savedErr; |
There was a problem hiding this comment.
Why a double loop here? Wouldn't it suffice to just grow from the shortest path (AKA root) towards all the sub directories? Also, with fstatat and mkdirat you even could leverage the work you need to do for splitting the path anyway …
There was a problem hiding this comment.
- I have considered the possibility of walking from the top level downwards. However, it can come with a different set of problems such as: I might not have permission to list or write to an upper level directory while writing to a lower level directory is okay. (One case of such a filesystem is in Termux.) Walking from a top level could mean I have to ignore errors all the way. I just prefer a safer route.
- Any API that uses
dirfdas arguments is out because the API for creating the final temp file, mkstemp(3), has no way to specify adirfdin. Surely it can be worked-around by using fchdir(2), but then you need a way to change back the working directory which is another pain to handle (and probably not worth it).
There was a problem hiding this comment.
- The mkdir(2) family of system calls in Unix has a problem that I would consider a design defect: There's no way to create and open the new directory (grabbing its
fd) in one atomic call. Yeah, that means trying to workaround a data race would be difficult (even though possible).
There was a problem hiding this comment.
I noticed the third issue, while looking at the manpage of mkdirat(2), which returns 0 on success instead of a dirfd …
And while you may not be allowed to list parent directories, you should still be allowed to open dirfd to any parent directories on the way, otherwise you wouldn't be able to walk the path to the child directory. And that's IMHO all you need to opendirat if it exists, fstatat to verify the subdir, and mkdirat otherwise.
Or expressed differently: If there is a directory /home/mystery/invisible/foo where you can't list /home/mystery, you still need to be able to confirm /home/mystery/invisible to exist: This can be either done by trying to create it (EEXIST on mkdir) or trying to opendir it. So, you either can't open the directory (EPERM vs. ENOENT) or the directory doesn exist and you just can't open it (EPERM).
This holds for the top-most directory that you need to create: If you can access a directory for reading you can determine if a certain entry inside that directory exists, even if you can't list that directory. This even holds true if you only obtain access to any child: If you can access any child, you know all of its parents MUST exist; thus no need to create them.
There was a problem hiding this comment.
And while you may not be allowed to list parent directories, you should still be allowed to open dirfd to any parent directories on the way, otherwise you wouldn't be able to walk the path to the child directory. And that's IMHO all you need to
opendiratif it exists,fstatatto verify the subdir, andmkdiratotherwise.
I need mkdirat and openat in one single operation, not two. If I check if directory exists using openat, and create the directory using mkdirat otherwise, there is a TOCTTOU, and I would rather give up trying to openat the thing.
Or expressed differently: If there is a directory
/home/mystery/invisible/foowhere you can't list/home/mystery, you still need to be able to confirm/home/mystery/invisibleto exist: This can be either done by trying to create it (EEXISTonmkdir) or trying toopendirit. So, you either can't open the directory (EPERMvs.ENOENT) or the directory doesn exist and you just can't open it (EPERM).
I don't feel it right to assume mkdir("/home/mystery/invisible", ...) will always return EEXIST. Even if I know the directory will exist, the kernel might as well return EACCSS or EPERM when it doesn't have the permission to even tell whether it exists or not.
(I don't know if any kernel does this yet, but if any does, I'll praise it as having a good security feature.)
This holds for the top-most directory that you need to create: If you can access a directory for reading you can determine if a certain entry inside that directory exists, even if you can't list that directory. This even holds true if you only obtain access to any child: If you can access any child, you know all of its parents MUST exist; thus no need to create them.
When it comes to the constraint that I have to "mkdir and open in one single operation", this part of the question is moot. I would call mkdir on all parent directories no matter what. And, as I mentioned above, it's not reliable to expect mkdir to say EEXIST for any directory that does exist.
| } | ||
|
|
||
| static int makeAncestorDirectories(char* filePath, size_t len) { | ||
| assert(len == strlen(filePath)); |
There was a problem hiding this comment.
Why assert on the length of the name, when you need to calculate it anyways?
Also, cf. note below, you can grow downwards from / to /home to /home/user etc. without knowing the length and have this work just the same.
There was a problem hiding this comment.
I've changed the algorithm to walk through the path from top down, so the question of string length is now moot.
ba6f54c to
38e0e08
Compare
|
We deliberate did not create the dirs in the past as this is a security / DOS risk. If
That should be good enough for the technically inclined that can use htop beyond showing hackers in movies. |
I don't think it makes sense not to create the directories. What kind of security risk can this bring? The same thing can be done with What I can see is exposing directories that have mode bit set wrong so that the user can spam things in them. |
2edeff7 to
443fd58
Compare
... and don't create config directories when reading the htop config. The new subroutine for creating directories can also work with custom paths specified in HTOPRC environment variable. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
No need to temporarily duplicate the XDG_CONFIG_HOME path string. Note that I didn't simplify or leave out the allocation of "$XDG_CONFIG_HOME/htop" or "$HOME/.config/htop" string because I suspect this string might be reused in the future. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
443fd58 to
3b4ea91
Compare
|
After some internal discussion and in light of a recently discovered bug that could be used to DoS certain applications by creating rogue directories in places where they don't belong, we decided to not merge this change. I cherry-picked the FD leak commit. The code shrink in 3b4ea91 looks interesting, but should best be handled in a separate PR. |
I would like to see how the bug can be done. For me, even when this PR is rejected, the current code of handling directories and writing the htoprc file is not ideal. If we want to prevent creation of "rogue directories", then we should also prevent creation of htoprc file in "places where they don't belong". Otherwise the behavior of preventing DoS would be inconsistent. |
|
@Explorer09 You got mail. HTH. For now I'd avoid changing the behavior regarding the config file. Please port the third commit as per my previous comment to a new PR. |
Improve the Settings_write() operation so that it can create directories just before creating and writing to the new htoprc file.
It can also work with a custom htoprc file path specified in
HTOPRCenvironment variable.With this improvement, htop will no longer create any config directory on reading the configuration; only on writing will the config directory be created.