Fix pidfile_open(3) for relative paths.

[markj]

Summary

The basename() usage added in r322369 is wrong and caused us to store
the full path in the filepath buffer. If the path is absolute or
consists of just a filename, this happens to work. But if the path is
relative and contains at least one directory component, the openat()
call will fail.

Fix this, and add a quick and dirty regression test.

[cem]

If the path is absolute or consists of just a filename, this happens to work. But if the path is relative and contains at least one directory component, the openat() call will fail.

Is that a sane use of the pidfile API? I guess it doesn't hurt to fix it, but usually pid files are absolute paths, I think.

[cem]

lib/libutil/pidfile.c

122–131 ↗

(On Diff #55504)

How is this different, aside from checking that the given name isn't longer than MAXPATHLEN?

filenamelen and dirlen are computed from the dirname/basename rather than the total path, but otherwise the logic seems to produce the same result? Just with more rototilling than necessary?

[cem]

lib/libutil/pidfile.c

122–131 ↗

(On Diff #55504)

Oh, I see. basename returns the correct value rather than rewriting the buffer entirely. So we have to copy the result over pf_filename. dirname was fine as-is, I think?

[markj]

In D19728#422602, @cem wrote:

If the path is absolute or consists of just a filename, this happens to work. But if the path is relative and contains at least one directory component, the openat() call will fail.

Is that a sane use of the pidfile API? I guess it doesn't hurt to fix it, but usually pid files are absolute paths, I think.

It doesn't seem very typical, but I don't think it's insane. I think one could easily hit the problem in a shell script that uses daemon(8) to run a program and stash its pidfile somewhere local.

lib/libutil/pidfile.c

122–131 ↗

(On Diff #55504)

In the old version, basename(pfh->pf_filename) has no side effects. With this change, we are copying the basename of the pidfile path into pfh->pf_filename.

[markj]

lib/libutil/pidfile.c

122–131 ↗

(On Diff #55504)

Yeah, I think the dirname() usage was ok. The new version does more copying than is strictly necessary; I was just trying to avoid depending on the internals of either dirname() or basename().

[cem]

LGTM

What about this makes it specific to relative paths? Wasn't it just entirely broken for any non-NULL path provided to pidfile_open?

lib/libutil/pidfile.c

122–131 ↗

(On Diff #55504)

Well, basename does have side-effects in terms of zeroing out '/' characters in the buffer. But it doesn't move the base component to the front of the buf as one of those side-effects. (Nitpicking, sorry)

Dirname rationale is reasonable to me.

[markj]

lib/libutil/pidfile.c

122–131 ↗

(On Diff #55504)

I thought it did that too, but it doesn't. For a path like "foo/bar" it leaves the string unmodified and returns a pointer to "bar".

[markj]

In D19728#422613, @cem wrote:

LGTM

What about this makes it specific to relative paths? Wasn't it just entirely broken for any non-NULL path provided to pidfile_open?

No, because the full path was stored unmodified in pf_filename. So openat(dirfd, <absolute path>) works (behaves the same as open(<absolute path>), and it also works on any path which is equal to its basename.