Re: [OE-core] [PATCH] systemd: Fix build on musl

["Luca Bocassi" <luca.boccassi@gmail.com>]

On Thu, 29 Jul 2021 at 20:11, Andre McCurdy <armccurdy@gmail.com> wrote:
>
> On Thu, Jul 29, 2021 at 6:49 AM Luca Bocassi <luca.boccassi@gmail.com> 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.

Kind regards,
Luca Boccassi