On Fri, Aug 6, 2021 at 7:12 AM Luca Boccassi <luca.boccassi@...> wrote:

On Thu, 29 Jul 2021 at 20:11, Andre McCurdy <armccurdy@...> wrote:

On Thu, Jul 29, 2021 at 6:49 AM Luca Bocassi <luca.boccassi@...> wrote:

Having a look at the patches, a few comments:

- 0012-don-t-pass-AT_SYMLINK_NOFOLLOW-flag-to-faccessat.patch I find
quite worrying, as it fundamentally changes access patterns, some of
which are done for security reasons. At best, this will cause
completely different runtime behaviours for the same filesystem
depending on the libc implementation, which doesn't sound great?

I wrote a long and verbose comment when I created the patch which
tries to document any differences in runtime behaviour.

----
Avoid using AT_SYMLINK_NOFOLLOW flag. It doesn't seem like the right thing to
do and it's not portable (not supported by musl). See:

http://lists.landley.net/pipermail/toybox-landley.net/2014-September/003610.html
http://www.openwall.com/lists/musl/2015/02/05/2

Note that laccess() is never passing AT_EACCESS so a lot of the discussion in
the links above doesn't apply. Note also that (currently) all systemd callers
of laccess() pass mode as F_OK, so only check for existence of a file, not
access permissions. Therefore, in this case, the only distiction between
faccessat() with (flag == 0) and (flag == AT_SYMLINK_NOFOLLOW) is the
behaviour for broken symlinks; laccess() on a broken symlink will succeed
with (flag == AT_SYMLINK_NOFOLLOW) and fail (flag == 0).

The laccess() macros was added to systemd some time ago and it's not clear if
or why it needs to return success for broken symlinks. Maybe just historical
and not actually necessary or desired behaviour?
----

If that comment is now out of date or something is missing then please
send a patch to update it.

However looking at this patch again now, it appears to have got broken
during a past rebase:

https://git.openembedded.org/openembedded-core/commit/?id=e8dd5a36bf2f1e645fb2ff15eb3b5e97c04776e6

The upstream code changed from:

#define laccess(path, mode) faccessat(AT_FDCWD, (path), (mode),
AT_SYMLINK_NOFOLLOW)

to

#define laccess(path, mode) \
(faccessat(AT_FDCWD, (path), (mode), AT_SYMLINK_NOFOLLOW) <
0 ? -errno : 0)

but the replacement version in the patch still returns the raw result
from faccessat(). That looks like an issue.

If you think the flag is unnecessary (I don't, we use these for a
reason, but that's not important right now), the correct action is to
send a PR upstream to discuss removing it. Patching it out for one
build case of many is just going to be a source of incompatibility and
surprises for users, as the behaviour on the same filesystem changes
depending on the build option. Having said that, I don't use musl so
all of this is really not a problem for me, just providing some
feedback as upstream maintainer, in case it can be useful.

I don't have any interest in systemd + musl anymore either. I did an
initial port as a proof of concept and sent patches to Khem off
list... and was somewhat surprised when they showed up some time later
in oe-core.

Note that there have been long discussions here previously about
whether OE should claim to support systemd + musl. The effort to
support it properly (including clarifying questions like this with
upstream as you suggest) doesn't seem huge but so far no one seems to
care enough about systemd + musl to do it. We rebase and tweak the
patches but guidance to potential users should still be "use at your
own risk".