Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Replace mkpath with sc_nonfatal_mkpath #193
Conversation
zyga
added some commits
Nov 24, 2016
| + * | ||
| + * This function behaves like "mkdir -p" (recursive mkdir) with the exception | ||
| + * that each directory is carefully created in a way that avoids symlink | ||
| + * attacks. The preceding directory is kept openat(2) (along with O_PATH) and |
|
Looks fine. I noticed that you call sc_nonfatal_mkpath() and then die() everytime if it fails. If the goal is to have better error handling mkpath() could also be made smarter. But its fine, I guess this will come useful we just need to make sure that we always handle the return value of mkpath() (there is an attribute for this iirc?). |
|
Good idea about using a gcc attribute for controlling the error code being used. I'll make that happen. As for the error message, I know I could have just passed in a string that gets pushed to die() but that felt uglier. |
zyga
added some commits
Nov 24, 2016
zyga
referenced this pull request
Nov 24, 2016
Closed
Fix creation of $XDG_RUNTIME_DIR and $SNAP_USER_DATA #195
zyga
added some commits
Nov 25, 2016
jdstrand
requested changes
Nov 28, 2016
•
In general, LGTM but please respond to the AT_FDCWD question. The /run/user/UID permissions could be fixed in a separate commit.
| if (errno == 0 && chmod(xdg_runtime_dir, 0700) != 0) { | ||
| - die("could not chmod XDG_RUNTIME_DIR"); | ||
| + die("cannot change permissions of user XDG_RUNTIME_DIR directory to 0700"); |
jdstrand
Nov 28, 2016
Contributor
Note there is a bug here that was present before this PR. /run/user/UID is supposed to be 0700. XDG_RUNTIME_DIR is set to /run/user/UID/snap.name. If /run/user/UID didn't exist and snap-confine created it, then it will be 0755. This could be solved by something along the lines of (note man 1 dirname for use of tmp):
// XDG_RUNTIME_DIR as set by 'snap run' is a subdirectory of the user's session
// XDG_RUNTIME_DIR (ie, /run/user/UID/snap.name). Since it is possible that the
// user's session XDG_RUNTIME_DIR (ie, /run/user/UID) does not exist yet, follow
// the freedesktop.org specification and ensure the session /run/user/UID is 0700
// when we create it ourselves, and also make sure the snap's XDG_RUNTIME_DIR
// is 0700.
char *tmp __attribute__ ((cleanup(sc_cleanup_string))) = NULL;
char *parent_dir __attribute__ ((cleanup(sc_cleanup_string))) = NULL;
tmp = strdup(xdg_runtime_dir);
parent_dir = dirname(tmp);
if (sc_nonfatal_mkpath(parent_dir, 0755) < 0) {
...
if (errno == 0 && chmod(parent_dir, 0700) != 0) {
...
if (sc_nonfatal_mkpath(xdg_runtime_dir, 0755) < 0) {
...
if (errno == 0 && chmod(xdg_runtime_dir, 0700) != 0) {
...
zyga
Nov 29, 2016
Collaborator
Yeah, I think you are right. Surfacing the mode argument made me realise that ownership and mode need to be proper for each step along the chain. I'll iterate on this with the next branch (I want to merge this task and the other related pull request into one follow-up branch)
jdstrand
Nov 29, 2016
Contributor
Sounds fine. This is a little trickier than I made out in this comment so best to leave for another PR.
| @@ -152,3 +156,63 @@ int must_snprintf(char *str, size_t size, const char *format, ...) | ||
| return n; | ||
| } | ||
| + | ||
| +int sc_nonfatal_mkpath(const char *const path, mode_t mode) |
jdstrand
Nov 28, 2016
Contributor
sc_nonfatal_mkpath seems like a weird name for a function. I guess you named it that so that users understand that they must check the return code? I guess that is ok and won't block on the name.
zyga
Nov 29, 2016
Collaborator
Yeah, I plan to unify all the names n the C code and this feels like one of the first ones that don't call di.e.()
| + char *path_segment = strtok_r(path_copy, "/", &path_walker); | ||
| + while (path_segment) { | ||
| + // Try to create the directory. It's okay if it already existed, but | ||
| + // any other error is fatal. Reset errno before attempting this as it |
jdstrand
Nov 28, 2016
Contributor
The other errors aren't fatal in terms of 'die()', they are just an error condition. Perhaps rephrase to "It's okay if it already existed, but return with error on any other error."
| + // previous one) so we can continue down the path. | ||
| + int previous_fd = fd; | ||
| + fd = openat(fd, path_segment, open_flags); | ||
| + if (previous_fd != AT_FDCWD && close(previous_fd) != 0) { |
jdstrand
Nov 28, 2016
Contributor
Can you explain the added previous_fd != AT_FDCWD check here? This seems like you'll return -1 as soon as you go two deep since previous_fd changed to fd but there are no chdir() calls and AT_FDCWD didn't change.
zyga
Nov 29, 2016
Collaborator
This is so that we don't try to close AT_FDCWD that we do close if the path is relative. I found this while writing the unit tests (I started with relative paths since those were easier to structure).
zyga
Nov 29, 2016
Collaborator
To make that clear. My reasoning is that fd starts with AT_FDCWD and that is moved to previous_fd in each iteration. We should never attempt to close that special value as it will always fail. Once we are out of the initial loop cycle we will never see that special value again.
jdstrand
Nov 29, 2016
Contributor
Ah, that makes a lot of sense. Thanks for the explanation! Can you add a brief comment? Eg:
// Open the parent directory we just made (and close the
// previous one (but not the special value AT_FDCWD) so
// we can continue down the path.
zyga commentedNov 24, 2016
This branch fixes a rather confusing error message that is printed by each failure of mkpath ("cannot create user data directory") and replaces it with context-aware error message driven by the particular code that used mkpath.
This branch is here to aid in discovering which of the mkpath calls fail when testing snapd against master version of snap-confine.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com