#394 getcpuflags_arm.c: static sigjmp_buf makes concurrent mpg123_new() crash on aarch64

[Yury Luneff]

Version: 1.33.7 (the same code is in trunk r5622), built with --with-cpu=aarch64, macOS arm64.

With OPT_MULTI on ARM, INT123_getcpuflags() runs on every handle creation
(INT123_frame_init_par -> wrap_getcpuflags). It installs a process-wide SIGILL
handler and probes NEON with sigsetjmp() on a static jmpbuf. When two threads
create handles at the same time, both write that one buffer.

On macOS arm64, sigsetjmp() saves the return address into the buffer, calls
sigprocmask, reloads it from the buffer and then calls _setjmp(). If the other
thread's _setjmp() writes the buffer in between, the first thread returns to a
garbage address. This shows up as Audacious crashing while it scans a music
library on two threads (EXC_BAD_ACCESS, pc=0x1, under MPG123Plugin::read_tag ->
mpg123_new).

NEON is mandatory on AArch64, so the probe can be skipped there:

--- a/src/libmpg123/getcpuflags_arm.c
+++ b/src/libmpg123/getcpuflags_arm.c
@@ -36,7 +36,10 @@

unsigned int INT123_getcpuflags(struct cpuflags* cf)
{
-#ifndef _M_ARM
+#if defined(aarch64)
+ // NEON is mandatory on AArch64. The SIGILL probe below uses a static jmpbuf and is not thread-safe.
+ cf->has_neon = 1;
+#elif !defined(_M_ARM)
struct sigaction act, act_old;
act.sa_handler = mpg123_arm_catch_sigill;
act.sa_flags = SA_RESTART;

This does not help 32-bit ARM builds (arm_multi), where the probe is still
needed and still races. Running the detection only once, for example with
pthread_once, would fix both.

Homebrew is carrying this patch for now:
https://github.com/Homebrew/homebrew-core/pull/324797

[Thomas Orgis]

Oh, fuck. Yeah, I said it.

Once you are in a multithreaded environment, the very idea of messing around with a signal handler triggered by the possibly illegal instruction is very, very bad. This is not what I intended with libmpg123 and also the move to get rid of mpg123_init(), which used to do something and is a no-op nowadays. Were this functions still part of the active API and clients expected to call it once before doing concurrent work, it could settle the CPU flags once in a safe context. But still, messing with signal handlers in the library is still bad. The non-concurrent mpg123_init() is not coming back (people had issues with that fragile requirement).

So, we can do away with the test on AARCH64 as NEON is guaranteed to be present. This is all that matters in practice, but leaves the (theoretical?) problem there for ARM32. Not sure how many systems there are with multi-core 32 bit ARMs were this will really bite you.

Now, there are other ways to check NEON support via OS API. But that differs for each platform. Sucks having to enumerate these (Linux, Android, ... Windows … and iOS doesn't offer it at all?!).

https://stackoverflow.com/questions/26701262/how-to-check-the-existence-of-neon-on-arm

Invoking pthread_once also doesn't fly, as libmpg123 stays clear of any use of a threading API itself. This is intentional to keep it compatible with any environment and not depend on any threading code for applications that are not multi-threaded.

This is a bit complicated with me not having convenient access to a 32 bit ARM to test. Would have to spin up some hardware emulation, I guess. The nice thing about the current code is that it is agnostic about the OS as long as it does signal handling. The bad thing is that it uses signal handling.

Some solution is needed for the 32 bit case.

[Thomas Orgis]

Note https://www.arm.com/architecture/cpu/r-profile … mentions „optional Arm Neon technology, “ on AArch64. Seems that we have to move to OS api and guesswork? Messing with signal handlers really, really won't work in a multithreaded host application.

[Yury Luneff]

Thanks for the quick look!

What about testing __ARM_NEON at compile time instead of __aarch64__? The compiler defines it only
when it is allowed to emit NEON instructions anywhere in the program (it does so on its own, e.g. when
auto-vectorizing), so a binary built with it set already cannot run without NEON, and the runtime probe
cannot find out anything new:

#if defined(__ARM_NEON)
    cf->has_neon = 1;
#elif !defined(_M_ARM)
    ... probe as before ...

With clang it is defined for default arm64 and for armv7 with -mfpu=neon, and not defined for
armv8-a+nosimd (an AArch64 core without NEON) or Debian armhf (vfpv3-d16). So the optional-NEON AArch64
case still gets the probe, and 32-bit builds that target NEON lose it too.

What remains are 32-bit builds without a NEON baseline. There the OS can answer without signals:
getauxval(AT_HWCAP) & HWCAP_NEON on Linux and Android, elf_aux_info(AT_HWCAP, ...) on FreeBSD,
IsProcessorFeaturePresent(PF_ARM_NEON_INSTRUCTIONS_AVAILABLE) on Windows, keeping the signal probe only
as the fallback for anything else.

The Homebrew PR now uses the __ARM_NEON variant: https://github.com/Homebrew/homebrew-core/pull/324797