From 539cc264fffdc1e42972c920ccb3253834303afb Mon Sep 17 00:00:00 2001 From: Weaselbot Date: Tue, 10 Nov 2026 15:15:53 -0500 Subject: [PATCH] Work around aarch64+ASan clang preserve_none miscompile clang miscompiles preserve_none on aarch64: a preserve_none callee uses x19 as its frame base without saving it, so a caller that also uses x19 as its frame base resumes with a clobbered frame (llvm/llvm-project#70577, llvm/llvm-project#177519). ASan's larger frames make it reliable: n_string2() is left with nulled locals and the parser segfaults on the valid document `""`. preserve_none is only an optimization for the musttail chain, so fall back to the default calling convention where it is not trustworthy: - Define PRESERVE_NONE only when musttail is available (without it the parse() trampoline calls every continuation with an ordinary call, and those calls returned WeaselJson_REJECT for `""`) and we are not building for aarch64 with ASan. - Stop annotating scan_string()/scan_string_impl() with it: n_string2() calls them with an ordinary call and keeps buf and bufEnd live across it, so they must preserve their caller's callee-saved registers. Add a regression test for the minimal valid documents, parsing through parser3::Parser3 directly so the continuations run under the flags of the test binary; it segfaults in the aarch64+ASan build without this change. Closes #78. --- src/parser3.h | 19 +++++++++++-------- src/preserve_none.h | 29 ++++++++++++++++++++++++++++- src/test.cpp | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 9 deletions(-) diff --git a/src/parser3.h b/src/parser3.h index f66ecd6..855223b 100644 --- a/src/parser3.h +++ b/src/parser3.h @@ -284,11 +284,15 @@ inline PRESERVE_NONE ContinuationStatus n_number(Parser3 *self, char *buf, } // Advance buf until double quote, backslash, invalid utf8, or codepoint < -// 0x20 +// 0x20. +// +// Not PRESERVE_NONE, unlike the continuations: n_string2() calls this with an +// ordinary (not tail) call and keeps buf and bufEnd live across it, so it must +// preserve its caller's callee-saved registers. +// https://git.weaselab.dev/weaselab/weaseljson/issues/78 template -inline PRESERVE_NONE ContinuationStatus scan_string_impl(Parser3 *self, - char *&buf, - char *bufEnd) { +inline ContinuationStatus scan_string_impl(Parser3 *self, char *&buf, + char *bufEnd) { const auto before = buf; // Advance buf past characters that transition the accept state to itself @@ -343,20 +347,19 @@ template __attribute__((target("avx2"))) ContinuationStatus scan_string_impl>(Parser3 *, char *&, char *); -__attribute__((target("default"))) inline PRESERVE_NONE ContinuationStatus +__attribute__((target("default"))) inline ContinuationStatus scan_string(Parser3 *self, char *&buf, char *bufEnd) { MUSTTAIL return scan_string_impl>( self, buf, bufEnd); } -__attribute__((target("avx2"))) inline PRESERVE_NONE ContinuationStatus +__attribute__((target("avx2"))) inline ContinuationStatus scan_string(Parser3 *self, char *&buf, char *bufEnd) { MUSTTAIL return scan_string_impl>( self, buf, bufEnd); } #else -inline PRESERVE_NONE ContinuationStatus scan_string(Parser3 *self, char *&buf, - char *bufEnd) { +inline ContinuationStatus scan_string(Parser3 *self, char *&buf, char *bufEnd) { MUSTTAIL return scan_string_impl>(self, buf, bufEnd); } #endif diff --git a/src/preserve_none.h b/src/preserve_none.h index e10a500..544af44 100644 --- a/src/preserve_none.h +++ b/src/preserve_none.h @@ -1,10 +1,37 @@ #pragma once +#include "musttail.h" + #ifndef __has_attribute #define __has_attribute(x) 0 #endif -#if __has_attribute(preserve_none) +// Clang reports AddressSanitizer through __has_feature, GCC through +// __SANITIZE_ADDRESS__. Normalize so the check below has one spelling. +#ifndef __SANITIZE_ADDRESS__ +#if defined(__has_feature) +#if __has_feature(address_sanitizer) +#define __SANITIZE_ADDRESS__ +#endif +#endif +#endif + +// preserve_none lets a continuation hand off to its successor without saving +// callee-saved registers, so it is only worth something when the hand-off is a +// tail call. clang also miscompiles it on aarch64: a preserve_none callee uses +// x19 as its frame base without saving it, so a caller that also uses x19 as +// its frame base resumes with a clobbered frame. ASan's larger frames make that +// reliable (the parser segfaults on the valid document `""`), and without +// musttail the parse() trampoline calls every continuation with an ordinary +// call. Use the default calling convention in both of those configurations. +// +// This is a workaround for the compiler bug, not a requirement of +// preserve_none. +// https://github.com/llvm/llvm-project/issues/70577 +// https://github.com/llvm/llvm-project/issues/177519 +// https://git.weaselab.dev/weaselab/weaseljson/issues/78 +#if __has_attribute(preserve_none) && defined(HAS_MUSTTAIL) && \ + !(defined(__aarch64__) && defined(__SANITIZE_ADDRESS__)) #define PRESERVE_NONE __attribute__((preserve_none)) #else #define PRESERVE_NONE diff --git a/src/test.cpp b/src/test.cpp index 8919ccf..e104a5f 100644 --- a/src/test.cpp +++ b/src/test.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -195,6 +196,30 @@ std::vector stridedCuts(std::string const &doc, size_t stride) { return cuts; } +// Like parseChunked(), but uses parser3::Parser3 directly instead of the public +// API, so that the continuations are compiled into this translation unit and +// run under the flags this binary was built with. A `stride` of 0 feeds the +// whole document at once. +WeaselJsonStatus parseDirect(std::string const &doc, size_t stride) { + auto callbacks = noopCallbacks(); + void *storage = ::operator new(parser3::Parser3::allocationSize(1024)); + auto *parser = new (storage) parser3::Parser3{&callbacks, nullptr, 1024, 0}; + std::string copy = doc; + size_t chunk = stride == 0 ? copy.size() : stride; + WeaselJsonStatus status = WeaselJson_AGAIN; + for (size_t i = 0; i < copy.size() && status == WeaselJson_AGAIN; + i += chunk) { + status = + parser->parse(copy.data() + i, int(std::min(chunk, copy.size() - i))); + } + if (status == WeaselJson_AGAIN) { + status = parser->parse(nullptr, 0); + } + parser->~Parser3(); + ::operator delete(storage); + return status; +} + } // namespace TEST_CASE("parser3") { @@ -245,6 +270,20 @@ TEST_CASE("parser3") { } } +// The shortest valid documents, which must parse the same however they are +// chunked. This doubles as a regression test for +// https://git.weaselab.dev/weaselab/weaseljson/issues/78: `""` is the smallest +// input that runs the n_string2() -> scan_string() -> scan_string_impl() +// chain, which segfaulted under clang + AddressSanitizer on aarch64 when +// scan_string() was annotated preserve_none and clobbered n_string2()'s frame. +TEST_CASE("minimal valid documents") { + for (std::string doc : {"\"\"", "[]", "{}"}) { + for (size_t stride : {size_t(0), size_t(1), size_t(2)}) { + CHECK(parseDirect(doc, stride) == WeaselJson_OK); + } + } +} + TEST_CASE("overflow state is sticky") { auto c = noopCallbacks(); // stackSize 3 is exactly big enough to hold reset()'s bootstrap, but too