diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 1d0533e4f0..9397c4b384 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1082,17 +1082,20 @@ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, b /* Just like openat(), but adds one thing: optionally returns whether we created the file anew or if * it already existed before. This is only relevant if O_CREAT is set without O_EXCL, and thus will - * shortcut to openat() otherwise */ - - if (!ret_newly_created) - return RET_NERRNO(openat(dirfd, pathname, flags, mode)); + * shortcut to openat() otherwise. + * + * Note that this routine is a bit more strict with symlinks than regular openat() is. If O_NOFOLLOW + * is not specified, then we'll follow the symlink when opening an existing file but we will *not* + * follow it when creating a new one (because that's a terrible UNIX misfeature and generally a + * security hole). */ if (!FLAGS_SET(flags, O_CREAT) || FLAGS_SET(flags, O_EXCL)) { fd = openat(dirfd, pathname, flags, mode); if (fd < 0) return -errno; - *ret_newly_created = FLAGS_SET(flags, O_CREAT); + if (ret_newly_created) + *ret_newly_created = FLAGS_SET(flags, O_CREAT); return fd; } @@ -1100,42 +1103,25 @@ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, b /* First, attempt to open without O_CREAT/O_EXCL, i.e. open existing file */ fd = openat(dirfd, pathname, flags & ~(O_CREAT | O_EXCL), mode); if (fd >= 0) { - *ret_newly_created = false; + if (ret_newly_created) + *ret_newly_created = false; return fd; } if (errno != ENOENT) return -errno; - /* So the file didn't exist yet, hence create it with O_CREAT/O_EXCL. */ - fd = openat(dirfd, pathname, flags | O_CREAT | O_EXCL, mode); + /* So the file didn't exist yet, hence create it with O_CREAT/O_EXCL/O_NOFOLLOW. */ + fd = openat(dirfd, pathname, flags | O_CREAT | O_EXCL | O_NOFOLLOW, mode); if (fd >= 0) { - *ret_newly_created = true; + if (ret_newly_created) + *ret_newly_created = true; return fd; } if (errno != EEXIST) return -errno; - /* Hmm, so now we got EEXIST? This can indicate two things. First, if the path points to a - * dangling symlink, the first openat() will fail with ENOENT because the symlink is resolved - * and the second openat() will fail with EEXIST because symlinks are not followed when - * O_CREAT|O_EXCL is specified. Let's check for this explicitly and fall back to opening with - * just O_CREAT and assume we're the ones that created the file. */ - - struct stat st; - if (fstatat(dirfd, pathname, &st, AT_SYMLINK_NOFOLLOW) < 0) - return -errno; - - if (S_ISLNK(st.st_mode)) { - fd = openat(dirfd, pathname, flags | O_CREAT, mode); - if (fd < 0) - return -errno; - - *ret_newly_created = true; - return fd; - } - - /* If we're not operating on a symlink, someone might have created the file between the first - * and second call to openat(). Let's try again but with a limit so we don't spin forever. */ + /* Hmm, so now we got EEXIST? Then someone might have created the file between the first and + * second call to openat(). Let's try again but with a limit so we don't spin forever. */ if (--attempts == 0) /* Give up eventually, somebody is playing with us */ return -EEXIST; diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index 3da3caf4ab..55202240d3 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -670,6 +670,9 @@ TEST(openat_report_new) { ASSERT_OK_ERRNO(symlinkat("target", tfd, "link")); fd = openat_report_new(tfd, "link", O_RDWR|O_CREAT, 0666, &b); + ASSERT_ERROR(fd, EEXIST); + + fd = openat_report_new(tfd, "target", O_RDWR|O_CREAT, 0666, &b); ASSERT_OK(fd); fd = safe_close(fd); ASSERT_TRUE(b); diff --git a/src/test/test-id128.c b/src/test/test-id128.c index a6ed640bd6..eb3bfec2bb 100644 --- a/src/test/test-id128.c +++ b/src/test/test-id128.c @@ -286,9 +286,8 @@ TEST(id128_at) { ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); ASSERT_OK(id128_write_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, id)); ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); - ASSERT_OK(id128_write_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, id)); - ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); - ASSERT_OK(id128_write_at(tfd, "etc2/hoge-id", ID128_FORMAT_PLAIN, id)); + ASSERT_ERROR(id128_write_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, id), EEXIST); + ASSERT_OK(id128_write_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, id)); /* id128_read_at() */ i = SD_ID128_NULL; /* Not necessary in real code, but for testing that the id is really assigned. */