From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14914 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general,gmane.comp.handhelds.openembedded.core Subject: OpenEmbedded systemd+musl patch status Date: Fri, 8 Nov 2019 13:43:37 -0500 Message-ID: <20191108184337.GF16318@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="135578"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.5.21 (2010-09-15) Cc: musl@lists.openwall.com, raj.khem@gmail.com To: openembedded-core@lists.openembedded.org Original-X-From: musl-return-14930-gllmg-musl=m.gmane.org@lists.openwall.com Fri Nov 08 19:43:58 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1iT9Eg-000Yyw-IQ for gllmg-musl@m.gmane.org; Fri, 08 Nov 2019 19:43:54 +0100 Original-Received: (qmail 13462 invoked by uid 550); 8 Nov 2019 18:43:51 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 13427 invoked from network); 8 Nov 2019 18:43:51 -0000 Content-Disposition: inline Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:14914 gmane.comp.handhelds.openembedded.core:129309 Archived-At: 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.