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.

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

/lib/rc/bin/checkpath is broken in most recent release #418

Closed
klausman opened this issue Apr 7, 2021 · 5 comments
Closed

/lib/rc/bin/checkpath is broken in most recent release #418

klausman opened this issue Apr 7, 2021 · 5 comments

Comments

@klausman
Copy link

klausman commented Apr 7, 2021

# ls -ld /var/lib/powerdns/
drwxr-xr-x 1 root root 88 Apr  7 19:21 /var/lib/powerdns/
 # /lib/rc/bin/checkpath --directory --mode 750 /var/lib/powerdns/
 * /var/lib/powerdns/: creating directory
 * checkpath: mkdirat: No such file or directory
#

Not sure what's going on here, so I straced it:

geteuid()                               = 0
getgid()                                = 0
brk(NULL)                               = 0x558e51af7000
brk(0x558e51b18000)                     = 0x558e51b18000
openat(0, "/", O_RDONLY)                = 3
openat(3, "var", O_RDONLY|O_NOFOLLOW|O_PATH) = 4
newfstatat(4, "", {st_mode=S_IFDIR|0755, st_size=132, ...}, AT_EMPTY_PATH) = 0
close(3)                                = 0
openat(4, "lib", O_RDONLY|O_NOFOLLOW|O_PATH) = 3
newfstatat(3, "", {st_mode=S_IFDIR|0755, st_size=648, ...}, AT_EMPTY_PATH) = 0
close(4)                                = 0
openat(3, "powerdns", O_RDONLY|O_NOFOLLOW|O_PATH) = 4
newfstatat(4, "", {st_mode=S_IFDIR|0755, st_size=88, ...}, AT_EMPTY_PATH) = 0
close(3)                                = 0
openat(4, "", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC) = -1 ENOENT (No such file or directory)

ltrace:

geteuid()                                                     = 0
getgid()                                                      = 0
strrchr("/lib/rc/bin/checkpath", '/')                         = "/checkpath"
getopt_long(5, 0x7fff76217098, "dDfFpm:o:sWChqVv", 0x55bbe0233ae0, nil) = 100
getopt_long(5, 0x7fff76217098, "dDfFpm:o:sWChqVv", 0x55bbe0233ae0, nil) = 109
strtoul(0x7fff762192d3, 0x7fff76216db0, 8, 0)                 = 488
getopt_long(5, 0x7fff76217098, "dDfFpm:o:sWChqVv", 0x55bbe0233ae0, nil) = -1
strrchr("/var/lib/powerdns/", '/')                            = "/"
malloc(4096)                                                  = 0x55bbe02352a0
vsnprintf("", 4096, "%s", 0x7fff76216cd0)                     = 0
openat(0, 0x55bbe0231081, 0, 1)                               = 3
strdup("/var/lib/powerdns/")                                  = 0x55bbe02362b0
strtok("/var/lib/powerdns/", "/")                             = "var"
strdup("var")                                                 = 0x55bbe02362d0
openat(3, 0x55bbe02362d0, 0x220000, 0x726176)                 = 4
fstat(4, 0x7fff76216ed0, 0x220000, 0)                         = 0
close(3)                                                      = 0
free(0)                                                       = <void>
strtok(nil, "/")                                              = "lib"
strdup("lib")                                                 = 0x55bbe02362f0
openat(4, 0x55bbe02362f0, 0x220000, 0x62696c)                 = 3
fstat(3, 0x7fff76216ed0, 0x220000, 0)                         = 0
close(4)                                                      = 0
free(0)                                                       = <void>
strtok(nil, "/")                                              = "powerdns"
strdup("powerdns")                                            = 0x55bbe0236310
openat(3, 0x55bbe0236310, 0x220000, 0x736e647265776f)         = 4
fstat(4, 0x7fff76216ed0, 0x220000, 0)                         = 0
close(3)                                                      = 0
free(0)                                                       = <void>
strtok(nil, "/")                                              = nil
free(0x55bbe02362b0)                                          = <void>
openat(4, 0x55bbe02352a0, 0xa0900, 1)                         = 0xffffffff
einfo(0x55bbe02310de, 0x7fff762192d7, -120, 0 * /var/lib/powerdns/: creating directory
)                = 42
umask(00)                                                     = 022
mkdirat(4, 0x55bbe02352a0, 488, 0x7f578884d277)               = -1
umask(022)                                                    = 00
__errno_location()                                            = 0x7f5788717b08
strerror(2)                                                   = "No such file or directory"
eerror(0x55bbe02310f5, 0x7fff762192b6, 0x7f57888e4b8c, -104 * checkpath: mkdirat: No such file or directory
)  = 49
+++ exited (status 1) +++
@klausman
Copy link
Author

klausman commented Apr 7, 2021

It's the trailing slash:

# /lib/rc/bin/checkpath --directory --mode 750 /var/lib/powerdns
 * /var/lib/powerdns: correcting mode

vs.

# /lib/rc/bin/checkpath --directory --mode 750 /var/lib/powerdns/
 * /var/lib/powerdns/: creating directory
 * checkpath: mkdirat: No such file or directory

@williamh
Copy link
Contributor

williamh commented Apr 8, 2021

@orlitzky Can you offer any suggestions that will work here?

@orlitzky
Copy link
Contributor

orlitzky commented Apr 8, 2021

It's the strtok that's causing a problem with the trailing slash, right?

POSIX actually says that we have to support that:

A pathname that contains at least one non-<slash> character and that ends with one or more trailing <slash> characters shall not be resolved successfully unless the last pathname component before the trailing <slash> characters names an existing directory or a directory entry that is to be created for a directory immediately after the pathname is resolved.

(emphasis mine)

Maybe the simplest thing to do is strip any trailing slashes off of the path if type == inode_dir and if the path contains at least one non-slash character.

@klausman
Copy link
Author

klausman commented Apr 8, 2021

Found another corner case:

# mkdir /tmp/foo
# cd /tmp/foo
# /lib/rc/bin/checkpath --directory /$(pwd)
 * //tmp/foo: creating directory
# /lib/rc/bin/checkpath --directory /$(pwd)
# ls
foo
# pwd
/tmp/foo
# 

Note how two leading / result in /tmp/foo/foo being created, despite the argument being just /tmp/foo

williamh added a commit that referenced this issue Apr 13, 2021
@Whissi
Copy link
Contributor

Whissi commented Apr 14, 2021

The bugfix for this regression caused an even bigger regression, see https://bugs.gentoo.org/782808.

My suggestion:

  1. Revert any checkpath changes since (and including commit b6fef59, i.e. revert basically all changes to src/rc/checkpath.c since 2020+ (see https://www.gentoofan.org/gentoo/misc/openrc-0.43.2-revert_checkpath_changes.patch) and release new OpenRC 0.43 without any of these checkpath fixes to not hold back any other improvements.
  2. Create a test suite and write tests for checkpath.
  3. Resume working on the checkpath fix.
  4. Once we have a checkpath which is passing tests, tag a new release with these changes.

PS: I am also wondering why commit 6219d87 doesn't show up for 0.43.2 tag but it was included in 0.43.2 release tarball...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants