fix: libexpr: Don't silently terminate on failure in mkFailed#16143
fix: libexpr: Don't silently terminate on failure in mkFailed#16143ncaq wants to merge 2 commits into
Conversation
`mkFailed` is `noexcept`, but the `Value::Failed` allocation inside it can actually throw: it allocates on the Boehm GC heap, and our OOM handler (`GC_set_oom_fn`) converts allocation failure into `std::bad_alloc`, contrary to the old comment in `.clang-tidy`, which claimed Boehm aborts rather than throws. When that happened, the exception hit the `noexcept` boundary and the process died in `std::terminate`, with no indication of what went wrong or which evaluation error was being recorded, which made the actual cause of such failures very hard to diagnose. Instead, catch everything inside `mkFailed`, print a diagnostic to stderr and abort. The error path uses only `fputs` with static strings, since the heap is exactly what just failed. Printing the original evaluation error is best-effort, because formatting it with functions like `BaseError::what` may itself allocate; the static message is printed first so that at least the context is never lost. Since `mkFailed` no longer lets an exception escape a `new` expression, this also fixes the only `bugprone-unhandled-exception-at-new` warning in the codebase, so re-enable that check in `.clang-tidy`. ref NixOS#16023 Assisted-by: claude-fable-5
|
This seems pretty ad hoc to me: it's a lot of complexity for just one function, while we have numerous |
|
Presumably the reason why this happens in |
|
Yeah, bad_alloc realistically is quite impossible to recover from when we need to allocate more stuff to report the error. Terminating / |
| /* Most likely a std::bad_alloc from the GC heap. | ||
| This is called while handling an evaluation error, | ||
| and recording that failure is itself what just failed, | ||
| so there is no way to recover. | ||
| Report and bail out, using only static strings to avoid touching the heap again. */ | ||
| std::fputs("error: failed to record an evaluation error", stderr); | ||
| try { | ||
| throw; | ||
| } catch (const std::exception & allocEx) { | ||
| std::fputs(": ", stderr); | ||
| std::fputs(allocEx.what(), stderr); | ||
| } catch (...) { | ||
| } | ||
| std::fputc('\n', stderr); | ||
|
|
||
| /* Best effort: show the evaluation error that was being recorded. | ||
| Formatting it (e.g. BaseError::what) may itself allocate and terminate on failure, | ||
| hence printing the message above first. */ | ||
| try { | ||
| std::rethrow_exception(e); | ||
| } catch (const std::exception & evalEx) { | ||
| std::fputs("the evaluation error was: ", stderr); | ||
| std::fputs(evalEx.what(), stderr); | ||
| std::fputc('\n', stderr); | ||
| } catch (...) { | ||
| std::fputs("the evaluation error was of an unknown type\n", stderr); | ||
| } | ||
|
|
||
| // Abort not exception but function. | ||
| std::abort(); |
There was a problem hiding this comment.
This seeems... very excessively bad. Can't we just call nix::panic? It doesn't try to allocate memory do it should be safe from the perspective.
There was a problem hiding this comment.
I'm newbie in nix repo.
I don't know nix:panic.
I will research it.
At least judging by the clang-tidy configuration comments, these appear to be the only instances triggering warnings for the use of |
The failure path of `mkFailed` printed the allocation exception by hand with `fputs`, and then called `std::abort` directly, which bypassed the crash handler and lost the stack trace. Call `panic()` instead. Its message is written with plain `write(2)` without touching the heap, and the `std::terminate()` it invokes runs the crash handler, which reports the allocation exception still active in the catch block, with its demangled type and message and a stack trace. This makes the manual printing of the allocation exception redundant, so drop it. The guarantees from the previous commit are preserved: a static message is printed first, so the context is never lost even if formatting the evaluation error terminates, and showing the evaluation error itself remains best-effort since `BaseError::what` may allocate. Assisted-by: claude-fable-5
|
I certainly wouldn't deny that this borders on an ad-hoc fix, |
|
Tbh I think that is the wrong approach. As @edolstra mentions handleEvalExceptionForThunk / handleEvalExceptionForApp is the better place to handle this. If we run into a bad_alloc we might not want to allocate an ExceptionRef at all but just continue unwinding and let the caller handle that. Though as always, exception handling in a low memory situation is bound to be very fragile (and c++ stdlib has statically pre allocated memory for use with exceptions specifically as a hack for this). |
3 participants
mkFailedisnoexcept,but the
Value::Failedallocation inside it can actually throw:it allocates on the Boehm GC heap,
and our OOM handler (
GC_set_oom_fn) converts allocation failure intostd::bad_alloc,contrary to the old comment in
.clang-tidy,which claimed Boehm aborts rather than throws.
When that happened,
the exception hit the
noexceptboundary and the process died instd::terminate,with no indication of what went wrong or which evaluation error was being recorded,
which made the actual cause of such failures very hard to diagnose.
Instead,
catch everything inside
mkFailed,print a diagnostic to stderr and abort.
The error path uses only
fputswith static strings,since the heap is exactly what just failed.
Printing the original evaluation error is best-effort,
because formatting it with functions like
BaseError::whatmay itself allocate;the static message is printed first so that at least the context is never lost.
Since
mkFailedno longer lets an exception escape anewexpression,this also fixes the only
bugprone-unhandled-exception-at-newwarning in the codebase,so re-enable that check in
.clang-tidy.ref #16023
Assisted-by: claude-fable-5