Some crashes on invalid data.

[fire]

Teachers have said that we should probably check pre-conditions before using GetLabels or DedupePropVerts, but it's hard to ensure the contract is correct.

I was able to cause manifold to crash modifying a text mesh from Godot to Godot is cool! with a union operation with a rectangular prism.

Not sure exactly how to proceed.

https://gist.github.com/31/c3c4bace42d4ca550ae6badf0f2b7cf9

From 9f7195298c356f841e7f2106b7b8cff4bccf3aa5 Mon Sep 17 00:00:00 2001
From: "K. S. Ernest (iFire) Lee" <ernest.lee@chibifire.com>
Date: Thu, 25 Jul 2024 00:39:32 -0700
Subject: [PATCH] Avoid crash.

---
 .../manifold/src/manifold/src/impl.cpp        | 35 ++++++++++++++++---
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/19_godot/thirdparty/manifold/src/manifold/src/impl.cpp b/19_godot/thirdparty/manifold/src/manifold/src/impl.cpp
index a284b56a3..aed5220ba 100644
--- a/19_godot/thirdparty/manifold/src/manifold/src/impl.cpp
+++ b/19_godot/thirdparty/manifold/src/manifold/src/impl.cpp
@@ -258,9 +258,20 @@ struct CheckCoplanarity {

 int GetLabels(std::vector<int>& components,
               const Vec<std::pair<int, int>>& edges, int numNodes) {
+  if (numNodes <= 0) {
+    return 0;
+  }
+
   UnionFind<> uf(numNodes);
-  for (auto edge : edges) {
+
+  for (const auto& edge : edges) {
     if (edge.first == -1 || edge.second == -1) continue;
+
+    if (edge.first < 0 || edge.first >= numNodes ||
+        edge.second < 0 || edge.second >= numNodes) {
+      continue;
+    }
+
     uf.unionXY(edge.first, edge.second);
   }

@@ -272,11 +283,25 @@ void DedupePropVerts(manifold::Vec<glm::ivec3>& triProp,
   ZoneScoped;
   std::vector<int> vertLabels;
   const int numLabels = GetLabels(vertLabels, vert2vert, vert2vert.size());
-
   std::vector<int> label2vert(numLabels);
-  for (size_t v = 0; v < vert2vert.size(); ++v) label2vert[vertLabels[v]] = v;
-  for (auto& prop : triProp)
-    for (int i : {0, 1, 2}) prop[i] = label2vert[vertLabels[prop[i]]];
+  for (size_t v = 0; v < vert2vert.size(); ++v) {
+    if (vertLabels[v] >= 0 && vertLabels[v] < numLabels) {
+      label2vert[vertLabels[v]] = v;
+    } else {
+      return;
+    }
+  }
+
+  for (auto& prop : triProp) {
+    for (int i : {0, 1, 2}) {
+      if (prop[i] >= 0 && prop[i] < vertLabels.size() &&
+          vertLabels[prop[i]] >= 0 && vertLabels[prop[i]] < numLabels) {
+        prop[i] = label2vert[vertLabels[prop[i]]];
+      } else {
+        return;
+      }
+    }
+  }
 }
 }  // namespace

[pca006132]

Thanks for catching this, it is not supposed to crash. Feel free to send your patch as a PR for reviews and discussions.

For DedupePropVerts, I don't think we should have vertLabels[v] >= numLabels? This sounds like a bug elsewhere.

[elalish]

And even better, can you show us your crash by making a corresponding TEST? I have no way to repro the issue from your description. You can use imported models by following this example: https://github.com/elalish/manifold/blob/master/test/boolean_complex_test.cpp#L948-L956

[elalish]

@fire I know you have precious little time, but is there any way to assign someone who does from your side so we can actually follow up on your issues? This fire-and-forget approach to issue generation isn't working super well for collaboration.

[fire]

I was working with someone on our side in a pull request, but we never got great minimum recreation projects for the bug that you requested.

One of the problems is the crash is on interactive use and we can't save the result.

The Godot Engine team on a whole is deep in trying to get Godot 4.3 released and we're on RC stage. So there's less will to try to review 4.4+ features

[fire]

See https://github.com/godotengine/godot/pull/94321#issuecomment-2261560092

[elalish]

Got it, thanks for the context. So this may not be a crash in Manifold at all? I'll leave it open for now, but we'll probably have to close as no repro eventually.

[PiCode9560]

I tested it. I think this change fixed it

[elalish]

Thanks - if it shows up again, feel free to ping us here to reopen this.