Merge pull request #9617 from 9999years/stack-overflow-segfault

Fix segfault on infinite recursion in some cases

(cherry picked from commit bf1b294bd81ca76c5ec9fe3ecd52196bf52a8300)
Change-Id: Id137541426ec8536567835953fccf986a3aebf16

1 files changed, 107 insertions, 4 deletions

diff --git a/src/libutil/error.cc b/src/libutil/error.cc
index 55c0a4846..36afb22ae 100644
--- a/src/libutil/error.cc
+++ b/src/libutil/error.cc
@@ -49,6 +49,32 @@ std::ostream & operator <<(std::ostream & str, const AbstractPos & pos)
     return str;
 }
 
+/**
+ * An arbitrarily defined value comparison for the purpose of using traces in the key of a sorted container.
+ */
+inline bool operator<(const Trace& lhs, const Trace& rhs)
+{
+    // `std::shared_ptr` does not have value semantics for its comparison
+    // functions, so we need to check for nulls and compare the dereferenced
+    // values here.
+    if (lhs.pos != rhs.pos) {
+        if (!lhs.pos)
+            return true;
+        if (!rhs.pos)
+            return false;
+        if (*lhs.pos != *rhs.pos)
+            return *lhs.pos < *rhs.pos;
+    }
+    // This formats a freshly formatted hint string and then throws it away, which
+    // shouldn't be much of a problem because it only runs when pos is equal, and this function is
+    // used for trace printing, which is infrequent.
+    return std::forward_as_tuple(lhs.hint.str(), lhs.frame)
+        < std::forward_as_tuple(rhs.hint.str(), rhs.frame);
+}
+inline bool operator> (const Trace& lhs, const Trace& rhs) { return rhs < lhs; }
+inline bool operator<=(const Trace& lhs, const Trace& rhs) { return !(lhs > rhs); }
+inline bool operator>=(const Trace& lhs, const Trace& rhs) { return !(lhs < rhs); }
+
 std::optional<LinesOfCode> AbstractPos::getCodeLines() const
 {
     if (line == 0)
@@ -184,6 +210,69 @@ static bool printPosMaybe(std::ostream & oss, std::string_view indent, const std
     return hasPos;
 }
 
+void printTrace(
+    std::ostream & output,
+    const std::string_view & indent,
+    size_t & count,
+    const Trace & trace)
+{
+    output << "\n" << "… " << trace.hint.str() << "\n";
+
+    if (printPosMaybe(output, indent, trace.pos))
+        count++;
+}
+
+void printSkippedTracesMaybe(
+    std::ostream & output,
+    const std::string_view & indent,
+    size_t & count,
+    std::vector<Trace> & skippedTraces,
+    std::set<Trace> tracesSeen)
+{
+    if (skippedTraces.size() > 0) {
+        // If we only skipped a few frames, print them out normally;
+        // messages like "1 duplicate frames omitted" aren't helpful.
+        if (skippedTraces.size() <= 5) {
+            for (auto & trace : skippedTraces) {
+                printTrace(output, indent, count, trace);
+            }
+        } else {
+            output << "\n" << ANSI_WARNING "(" << skippedTraces.size() << " duplicate frames omitted)" ANSI_NORMAL << "\n";
+            // Clear the set of "seen" traces after printing a chunk of
+            // `duplicate frames omitted`.
+            //
+            // Consider a mutually recursive stack trace with:
+            // - 10 entries of A
+            // - 10 entries of B
+            // - 10 entries of A
+            //
+            // If we don't clear `tracesSeen` here, we would print output like this:
+            // - 1 entry of A
+            // - (9 duplicate frames omitted)
+            // - 1 entry of B
+            // - (19 duplicate frames omitted)
+            //
+            // This would obscure the control flow, which went from A,
+            // to B, and back to A again.
+            //
+            // In contrast, if we do clear `tracesSeen`, the output looks like this:
+            // - 1 entry of A
+            // - (9 duplicate frames omitted)
+            // - 1 entry of B
+            // - (9 duplicate frames omitted)
+            // - 1 entry of A
+            // - (9 duplicate frames omitted)
+            //
+            // See: `tests/functional/lang/eval-fail-mutual-recursion.nix`
+            tracesSeen.clear();
+        }
+    }
+    // We've either printed each trace in `skippedTraces` normally, or
+    // printed a chunk of `duplicate frames omitted`. Either way, we've
+    // processed these traces and can clear them.
+    skippedTraces.clear();
+}
+
 std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool showTrace)
 {
     std::string prefix;
@@ -332,7 +421,13 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
 
     bool frameOnly = false;
     if (!einfo.traces.empty()) {
+        // Stack traces seen since we last printed a chunk of `duplicate frames
+        // omitted`.
+        std::set<Trace> tracesSeen;
+        // A consecutive sequence of stack traces that are all in `tracesSeen`.
+        std::vector<Trace> skippedTraces;
         size_t count = 0;
+
         for (const auto & trace : einfo.traces) {
             if (trace.hint.str().empty()) continue;
             if (frameOnly && !trace.frame) continue;
@@ -342,14 +437,21 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
                 break;
             }
 
+            if (tracesSeen.count(trace)) {
+                skippedTraces.push_back(trace);
+                continue;
+            }
+            tracesSeen.insert(trace);
+
+            printSkippedTracesMaybe(oss, ellipsisIndent, count, skippedTraces, tracesSeen);
+
             count++;
             frameOnly = trace.frame;
 
-            oss << "\n" << "… " << trace.hint.str() << "\n";
-
-            if (printPosMaybe(oss, ellipsisIndent, trace.pos))
-                count++;
+            printTrace(oss, ellipsisIndent, count, trace);
         }
+
+        printSkippedTracesMaybe(oss, ellipsisIndent, count, skippedTraces, tracesSeen);
         oss << "\n" << prefix;
     }
 
@@ -368,4 +470,5 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
 
     return out;
 }
+
 }