Merge branch 'topic/etyp/recursive-type-segfault-fix'

CHANGES

@@ -1,3 +1,24 @@
+1.12.0-dev.127 | 2024-10-05 13:21:27 +0200
+
+  * GH-1867: Fix infinite loops with recursive types. (Evan Typanski, Corelight)
+
+    Closes #1867
+
+    There are two different cases where infinite loops happen with recursive
+    types. First, a type may reference itself (`type Data = Data`). Second,
+    a type may reference itself inside some other type (`type Data =
+    vector<Data>`).
+
+    The first is fixed with a recursion limit. Since the type simply cannot
+    resolve, it doesn't get anywhere near codegen. You could detect cycles,
+    but that introduces some extra overhead and complexity that shouldn't
+    be needed in a "simple" function.
+
+    The second is fixed with an ad-hoc "occurs" check in type unification.
+    That just detects cycles and aborts if one is present. This could be
+    placed at some other place in the "resolve until convergence" loop, but
+    it seems best put closest to the source of the issue.
+
 1.12.0-dev.124 | 2024-09-30 17:43:41 +0200
 
   * GH-1875: Fix potential nullptr dereference when comparing streams. (Robin Sommer, Corelight)

VERSION

@@ -1 +1 @@
-1.12.0-dev.124
+1.12.0-dev.127

hilti/toolchain/include/ast/types/name.h

@@ -2,8 +2,6 @@
 
 #pragma once
 
-#include <memory>
-#include <string>
 #include <utility>
 
 #include <hilti/ast/ast-context.h>
@@ -19,13 +17,16 @@ class Name : public UnqualifiedType {
     bool isBuiltIn() const { return _builtin; }
 
     // resolves recursively
-    UnqualifiedType* resolvedType() const {
+    UnqualifiedType* resolvedType(size_t recursion_depth = 0) const {
         if ( ! _resolved_type_index )
             return nullptr;
 
+        if ( recursion_depth > 1000 )
+            return nullptr;
+
         auto t = context()->lookup(_resolved_type_index);
         if ( auto n = t->tryAs<type::Name>() )
-            return n->resolvedType();
+            return n->resolvedType(recursion_depth + 1);
         else
             return t;
     }

hilti/toolchain/include/compiler/type-unifier.h

@@ -5,6 +5,7 @@
 #include <string>
 
 #include <hilti/ast/forward.h>
+#include <hilti/ast/node.h>
 
 namespace hilti::type_unifier {
 
@@ -55,12 +56,14 @@ class Unifier {
     /** Resets all state to start a new unification. */
     void reset() {
         _serial.clear();
+        _cd.clear();
         _abort = false;
     }
 
 private:
-    std::string _serial; // builds up serialization incrementally
-    bool _abort = false; // if true, cannot compute serialization yet
+    std::string _serial;     // builds up serialization incrementally
+    node::CycleDetector _cd; // used to check for invalid cycles
+    bool _abort = false;     // if true, cannot compute serialization yet
 };
 
 namespace detail {

hilti/toolchain/src/compiler/resolver.cc

@@ -66,7 +66,10 @@ struct VisitorPass1 : visitor::MutatingPostOrder {
         }
 
         if ( n->resolvedTypeIndex() ) {
-            if ( auto resolved = n->resolvedType(); resolved->isOnHeap() ) {
+            auto resolved = n->resolvedType();
+            if ( ! resolved )
+                n->addError(util::fmt("type '%s' cannot be resolved by its name", n->id()));
+            else if ( resolved->isOnHeap() ) {
                 if ( auto qtype = n->parent()->tryAs<QualifiedType>() ) {
                     auto replace = false;
 

hilti/toolchain/src/compiler/type-unifier.cc

@@ -1,7 +1,5 @@
 // Copyright (c) 2020-2023 by the Zeek Project. See LICENSE for details.
 
-#include <optional>
-
 #include <hilti/ast/ast-context.h>
 #include <hilti/ast/builder/builder.h>
 #include <hilti/ast/type.h>
@@ -203,9 +201,18 @@ class VisitorTypeUnifier : public visitor::MutatingPostOrder {
 } // namespace
 
 void type_unifier::Unifier::add(UnqualifiedType* t) {
+    // Occurs check: We cannot handle recursive types. Error out if we see the same
+    // node twice.
+    if ( _cd.haveSeen(t) ) {
+        t->addError(util::fmt("cycle detected in definition of type '%s'", t->typeID()));
+        abort();
+    }
+
     if ( _abort )
         return;
 
+    _cd.recordSeen(t);
+
     if ( auto name = t->tryAs<type::Name>() ) {
         t = name->resolvedType();
         if ( ! t ) {

tests/Baseline/hilti.types.id.recursion-limit/output

@@ -0,0 +1,7 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+[error] <...>/recursion-limit.sh.hlt:2:14-2:18: type 'Data1' cannot be resolved by its name
+[error] <...>/recursion-limit.sh.hlt:3:14-3:18: type 'Data2' cannot be resolved by its name
+[error] <...>/recursion-limit.sh.hlt:4:14-4:18: type 'Data3' cannot be resolved by its name
+[error] <...>/recursion-limit.sh.hlt:5:14-5:18: type 'Data4' cannot be resolved by its name
+[error] <...>/recursion-limit.sh.hlt:6:14-6:18: type 'Data5' cannot be resolved by its name
+[error] hiltic: aborting after errors

tests/Baseline/hilti.types.id.recursive/output

@@ -0,0 +1,7 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+[error] <...>/recursive.hlt:6:15-6:20: type 'Direct' cannot be resolved by its name
+[error] <...>/recursive.hlt:7:19-7:40: cycle detected in definition of type 'Test::Referenced'
+[error] <...>/recursive.hlt:8:17-8:32: cycle detected in definition of type 'Test::InVector'
+[error] <...>/recursive.hlt:10:14-10:19: type 'Second' cannot be resolved by its name
+[error] <...>/recursive.hlt:11:15-11:19: type 'First' cannot be resolved by its name
+[error] hiltic: aborting after errors

tests/hilti/types/id/recursion-limit.sh

@@ -0,0 +1,28 @@
+#!/bin/sh
+#
+# Tests to make sure deeply nested types error out before they cause an overflow
+#
+# @TEST-EXEC: /bin/sh %INPUT %INPUT.hlt
+# @TEST-EXEC-FAIL: ${HILTIC} -p %INPUT.hlt > output 2>&1
+# @TEST-EXEC: btest-diff output
+
+filename=$1
+NUM_ITERATIONS=1005
+
+if test $# -ne 1; then
+    echo >&2 "No filename provided"
+    exit 1
+fi
+
+echo "module Overflow {" >> "$filename"
+
+i=0
+while [ $i -le $NUM_ITERATIONS ]; do
+    echo "type Data$i = Data$((i+1));" >> "$filename"
+    i=$((i+1))
+done
+
+# Doesn't matter, just make it resolve
+echo "type Data$i = uint<8>;" >> "$filename"
+
+echo "}" >> "$filename"

tests/hilti/types/id/recursive.hlt

@@ -0,0 +1,13 @@
+# @TEST-EXEC-FAIL: ${HILTIC} -p %INPUT > output 2>&1
+# @TEST-EXEC: btest-diff output
+
+module Test {
+
+type Direct = Direct;
+type Referenced = strong_ref<Referenced>;
+type InVector = vector<InVector>;
+
+type First = Second;
+type Second = First;
+
+}