[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191108184337.GF16318@brightrain.aerifal.cx>
Date: Fri, 8 Nov 2019 13:43:37 -0500
From: Rich Felker <dalias@...c.org>
To: openembedded-core@...ts.openembedded.org
Cc: musl@...ts.openwall.com, raj.khem@...il.com
Subject: OpenEmbedded systemd+musl patch status
Some questions came up on #musl about systemd and musl
[non-]compatibility, and whether any of the patches being applied are
problematic/buggy, so I'm doing a quick review of my take on each,
from the list here:
https://git.openembedded.org/openembedded-core/tree/meta/recipes-core/systemd/systemd_243.bb#n31
https://git.openembedded.org/openembedded-core/tree/meta/recipes-core/systemd/systemd
0002-don-t-use-glibc-specific-qsort_r.patch
This looks ok-ish for now. It's not "musl specific" because qsort_r is
not a standard function and historical implementations differ in the
argument order, which is why musl has it -- lack of consensus over
something like this is a strong criterion for exclusion. I've been
working to get consensus on adopting the glibc argument order, and BSD
folks seem onboard with changing to match glibc and proposing qsort_r
for standardization in POSIX, so I think we'll support it in the
future.
In the mean time, to be safe the globals used to replace the context
argument should have __thread storage duration; current code in the
patch is silently thread-unsafe.
0003-missing_type.h-add-__compare_fn_t-and-comparison_fn_.patch
Harmless but slightly wrong, and not "musl specific" as described.
It has undefined behavior by virtue of defining (and systemd already
had UB by using) identifier in the reserved __ namespace. The correct
fix here is for systemd not to use the gratuitous glibc typedefs for
this function type and instead just write the type out, or make its
own typedef (by writing the type out) that doesn't involve the
gratuitous glibc typedef.
0004-add-fallback-parse_printf_format-implementation.patch
I don't understand what systemd wants with this, but by itself it's
harmless or at least doesn't have any obvious errors. This is not
"musl-specific". It's providing a fallback for a highly glibc-specific
interface.
0005-src-basic-missing.h-check-for-missing-strndupa.patch
strndupa has been proposed for musl and will probably be added at some
point (it's header-only and painless), but its use is almost always
insecure and a serious code smell, so it would be worth looking into
why systemd is using it...
0006-Include-netinet-if_ether.h.patch
Documented musl behavior is that if you want to use both libc headers
and kernel headers that cover the same types, the libc ones must be
included first so that they can suppress the kernel definitions with
the appropriate UAPI macros. We don't support the other direction
(using kernel-defined UAPI macros to suppress libc definitions) for
several reasons, probably the most important of which is that the
kernel ones may be using types that align in ABI layout but that are
not C-language-level compatible types (kernel folks don't seem to care
about that) and might break things elsewhere.
You can call this "musl-specific" if you like, but use of kernel
headers that clash with libc ones is not well-defined to begin with,
and AIUI the glibc folks are in agreement that the way musl supports
is preferable (less likely to break from kernel doing wacky things)
even though they support both. So I think changing the order makes
sense independent of musl.
0007-don-t-fail-if-GLOB_BRACE-and-GLOB_ALTDIRFUNC-is-not.patch
These are both glibc-specific extensions which applications are
supposed to test for and provide their own copy of GNU glob (e.g. from
gnulib) if the system glob does not support them (or just do without).
The patch as proposed takes the "do without" option.
GLOB_ALTDIRFUNC has been proposed for musl and possibly meets
conditions for inclusion (it's iff) but the current proposed patch
introduces UB, and it's a pain to fix it not to introduce UB.
GLOB_BRACE has not been investigated yet.
The systemd patch is probably ok but might have user-facing effects.
0008-add-missing-FTW_-macros-for-musl.patch
This patch is bad, defining macros for functionality that's not
supported. musl should probably be returning an error when it sees
these unsupported bits but it's probably silently ignoring them.
Neither is likely to be helpful. If they're really needed, an alt
implementation of nftw is probably called for here (unless these
macros make sense in musl; I don't think this has been evaluated).
I'm not sure how harmful this patch is as-is but it's not correct.
0010-fix-missing-of-__register_atfork-for-non-glibc-build.patch
This is abvolutely not "musl specific". The comment in the systemd
source file being patched is clear that the thing here is an awful
hack for avoiding needing -lpthread with glibc. It's poking at a
glibc-internal symbol in __ namespace and has UB. It should just be
removed and pthread_atfork should always be used.
However note that most uses of pthread_atfork have UB, since the child
context after a multithreaded process forks is in an async-signal
context where you cannot use most functions, including locking
functions (except sem_post which is AS-safe), and even if you could,
it would be UB to unlock a lock that you took in the parent in the
child, since the child is not the owner and unlock by non-owner has
UB. This was a design bug in the POSIX threads spec that's been
acknowledged. So systemd probably has bugs here.
0011-Use-uintmax_t-for-handling-rlim_t.patch
This is a fix that should have been accepted upstream. Assuming rlim_t
has same type as uint64_t is wrong.
0014-test-sizeof.c-Disable-tests-for-missing-typedefs-in-.patch
This patch is correct, and the removal of these tests need not even be
dependent on !__GLIBC__. The types being checked are internal types
for use by glibc's headers, not public types provided by glibc.
0015-don-t-pass-AT_SYMLINK_NOFOLLOW-flag-to-faccessat.patch
This patch looks correct, but I'm not sure if there's more behind the
laccess mess systemd is doing. AT_SYMLINK_NOFOLLOW is not specified as
a valid flag for faccessat, and the kernel does not offer any direct
way to perform such an action. I'm not sure what hackery glibc is
doing to provide it, but I wouldn't be surprised if it has buggy
corner cases. We specifically punted on it unless/until it's required
by the spec, since it's likely hard or impossible to make it work
right.
0016-Define-glibc-compatible-basename-for-non-glibc-syste.patch
This patch is correct and not "musl specific". Rather, the incorrect
definition of basename systemd expects is glibc-specific, contrary to
the spec, and contrary to every other implementation.
0017-Do-not-disable-buffering-when-writing-to-oom_score_a.patch
I don't understand why this patch is needed, and I don't think it's
the right fix. There's probably an underlying bug in systemd behind
it.
0018-distinguish-XSI-compliant-strerror_r-from-GNU-specif.patch
Exact same issue as 0016 -- glibc has a wrong definition of a standard
function and systemd is relying on it.
0019-Hide-__start_BUS_ERROR_MAP-and-__stop_BUS_ERROR_MAP.patch
No idea why this is listed with musl+systemd patches. It does not seem
to have anything to do with musl.
0020-missing_type.h-add-__compar_d_fn_t-definition.patch
This patch is wrong but systemd is wrong too. It just should not be
using these gratuitous typedefs for comparator functions, which have a
standard type expressable without any system-header-provided typedefs
whatsoever. If they want a typedef they should define their own that's
not overlapping with one defined by glibc, and not in the reserved __
namespace.
0021-avoid-redefinition-of-prctl_mm_map-structure.patch
Not sure what the right thing to do here is. This should be detected
rather than using __GLIBC__ I think, if glibc actually needs it, but
it's messy. In any case the patch is harmless and not incorrect so
fine to keep for now.
0001-do-not-disable-buffer-in-writing-files.patch
Looks like #0017 above, probably same underlying issue. Patch is
almost surely not correct and there's likely an underlying bug/UB in
systemd that needs fixing.
0002-src-login-brightness.c-include-sys-wait.h.patch
Looks fine.
0003-src-basic-copy.c-include-signal.h.patch
Looks fine.
0004-src-shared-cpu-set-util.h-add-__cpu_mask-definition.patch
OK-ish, but defining this identifier in reserved namespace is wrong.
Rather, systemd should be fixed not to be poking at cpuset internals
but should correctly use accessor macros.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
