On Thu, 29 Jul 2021 at 20:11, Andre McCurdy <[email protected]> wrote: > > On Thu, Jul 29, 2021 at 6:49 AM Luca Bocassi <[email protected]> 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
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#154565): https://lists.openembedded.org/g/openembedded-core/message/154565 Mute This Topic: https://lists.openembedded.org/mt/84490599/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
