`coerceToString` can overflow the stack, the bad way

[roberth]

Describe the bug

Haven't spotted this in the wild, but:

nix-repl> let x = { outPath = x; }; in "${x}"
error: stack overflow (possible infinite recursion)

Of course here there's little a trace would add, but a real-world case doesn't have the luxury of a single line.

Steps To Reproduce

As above. Maybe add some fluff to make it realistic.

Expected behavior

Print positions, stack trace. Leverage the language call stack #9617
Perhaps use a different counter, because these are technically not calls in the language.
Tail recursion or a loop seem feasible, but over-engineered, and a limit is desirable in practice.

nix-env --version output

2.21

Additional context

Any C++-level recursion is susceptible. It'd be great to audit the code base using a static analysis tool. Doesn't have to run in CI in my opinion, so a tool with false positives is ok.

Priorities

Add 👍 to issues you find important.

[thufschmitt]

For that specific case, it seems that we could just detect the recursion with blackhole-ing?

In the general case that might not be possible (or maybe it is?), so yeah, some nicer stacktrace would be very useful

[roberth]

This recursion is productive, whereas blackhole works for recursions that aren't productive.

Unnecessary detail:
In fact, no actual evaluation happens after the first outPath is done. It's just C++ looping through a cyclic data structure. However, that doesn't really matter, because we may as well construct a diverging variation that produces new outPaths, and get the same behavior, which does involve a bit of evaluation. Maybe that might trigger the call limiter, but that's not a solution.

[thufschmitt]

This recursion is productive, whereas blackhole works for recursions that aren't productive.

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index bbccfcd29..02cd968ea 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -2327,6 +2327,7 @@ BackedStringView EvalState::coerceToString(
                 .withTrace(pos, errorCtx)
                 .debugThrow();
         }
+        v.mkBlackhole();
         return coerceToString(pos, *i->value, context, errorCtx,
                               coerceMore, copyToStore, canonicalizePath);
     }

$ nix eval --expr 'let x = { outPath = x; }; in "${x}"'                
error: infinite recursion encountered
       at «string»:1:31:
            1| let x = { outPath = x; }; in "${x}"
             | 

It technically is.

It might not be fully correct (it might leave some unwanted blackholes around), but something like the value printer who keeps a track of the values already seen would.

[roberth]

Interesting. Maybe all it needs to do is restore afterwards, like forceValue does.