ensure_file() uses stat() instead of lstat(), making S_ISLNK check dead code

[dwahdany]

ensure_file() in utils.c intends to reject symlinks as bind-mount destinations — the comment says "non-directory, non-symlink is acceptable" and the code checks !S_ISLNK(buf.st_mode).

However, it uses stat() which follows symlinks. Since stat() resolves the symlink before returning, S_ISLNK() is never true for valid symlinks, making the check dead code.

bubblewrap/utils.c

Lines 513 to 519 in 0c408e1

	We're trying to set up a mount point for a non-directory, so any
	non-directory, non-symlink is acceptable - it doesn't necessarily
	have to be a regular file. */
	if (stat (path, &buf) == 0 &&
	!S_ISDIR (buf.st_mode) &&
	!S_ISLNK (buf.st_mode))
	return 0;

Should use lstat() so the S_ISLNK check works as intended.

[smcv]

ensure_file() in utils.c intends to reject symlinks as bind-mount destinations

Is that really what is intended, though? At the moment, I can do this:

$ cd ~/tmp
$ touch file
$ ln -s file symlink-to-file
$ bwrap --dev-bind / / --bind /etc/os-release $(pwd)/symlink-to-file -- bash -i

and bwrap (or in fact the kernel) will traverse the symlink and mount /etc/os-release onto ~/tmp/file. What you've proposed in #734 would regress this, I think.

Since stat() resolves the symlink before returning, S_ISLNK() is never true for valid symlinks

You say "for valid symlinks", but the word "valid" is doing more heavy lifting there than you might imagine. There is one situation in which S_ISLNK() will be true for the result of stat(), and that's a dangling symlink.

At the moment, bubblewrap will (try to) create the target of the symlink, which is maybe not what anyone expected, but is somewhat consistent with how it will automatically create a regular file to act as a mount point if you try to mount onto a nonexistent path directly:

$ cd ~/tmp
$ rm -f nonexistent
$ ln -s nonexistent dangling-symlink
$ bwrap --dev-bind / / --bind /etc/os-release $(pwd)/dangling-symlink -- true
$ ls -l nonexistent
(exists)

The original implementation of ensure_file() in c3dd820 would try to create a regular file even if the path already existed as a socket or fifo or whatever, but that meant that --bind couldn't bind-mount sockets, which I fixed in a00f39f.

[smcv]

Is there a user-facing problem - like a command that should succeed but fails, or should fail but succeeds - that you are aiming to fix here? Or was this just "this doesn't look right"?

[smcv]

I think the confusion here is that you're seeing this check as being whether to accept or reject a mount point, but at the end of the day that's always going to be a decision for the kernel to make. The purpose of this check is more narrow: it's deciding whether there is a pre-existing filesystem object that would be suitable for us to mount a non-directory onto.

That's done because if there is not, we want to create one (or fail with an error if the kernel tells us that we can't). But if there is already a suitable filesystem object, we don't want to try to create one, because trying to create one might fail with errors like EROFS, even if a suitable object already exists and the mount would have succeeded. That's what the comment about "otherwise the create file will fail in the read-only case" is saying.

[dwahdany]

Or was this just "this doesn't look right"?

Yes it just looked like something was intended here that doesn't do what it's supposed to.

Is that really what is intended, though?
I think the confusion here is that you're seeing this check as being whether to accept or reject a mount point
There is one situation in which S_ISLNK() will be true for the result of stat(), and that's a dangling symlink.

So I think all is working as intended and what looked like a minor bug is actually intended and the proposed fix would wreck havoc instead of achieving anything :)

Thanks for the explanation! And sorry for the false alarm.

[smcv]

The comments were less clear than they should have been, so I can understand why it wasn't obvious. I've opened #737 to try to address that.