From 33b70be027e80ccc2e1c36dd60db32791a7f34ef Mon Sep 17 00:00:00 2001
From: Kaylee Lubick <kjlubick@google.com>
Date: Mon, 18 May 2026 16:07:23 +0000
Subject: [PATCH] Fix pathops bug with linked lists in SkOpCoincidence

I added some logging to SkOpCoincidence::fixUp and ::release and,
with the new test case (which has multiple coincidences [1]), observed

```
fixUp: release(0x...7378, 0x...7378)
release: head_arg=0x...7378, remove=0x...7378, global fHead=...7378
release: updated global fHead to 0x...72b8
fixUp: after release global fHead=0x...72b8
fixUp: release(0x...7378, 0x...6ec8)
```

As per the linked bug, this leads to a faulty state because the
old head pointer 0x...7378 is being used after it was "released".
I could not get ASAN to fire on this but it is worth fixing for
correctness and consistency.

My change fixes that particular issue and adds some asserts to avoid
this happening again.

I also rewrote some do while loops to
avoid iffy behavior where we were releasing coin and then for
the next iteration still calling coin->next(). This worked because
we had an SkArenaAlloc and that memory wasn't "fully" released.

[1] a coincidence is an overlap between two path segments that
lasts for more than a single point. e.g. two squares sharing an edge.

Bug: https://issues.chromium.org/issues/513001309
Fixed: 513001309
Change-Id: Ie3f6cea575acf9575e984b2b0d2eb788bb4b2052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1238176
Reviewed-by: Thomas Smith <thomsmit@google.com>
Commit-Queue: Kaylee Lubick <kjlubick@google.com>
---
 src/pathops/SkOpCoincidence.cpp | 94 +++++++++++++++++----------------
 src/pathops/SkOpCoincidence.h   |  8 +--
 4 files changed, 88 insertions(+), 63 deletions(-)

diff --git a/src/pathops/SkOpCoincidence.cpp b/src/pathops/SkOpCoincidence.cpp
index 4a8bcec1d8..a8a604ed36 100644
--- a/src/pathops/SkOpCoincidence.cpp
+++ b/src/pathops/SkOpCoincidence.cpp
@@ -695,8 +695,8 @@ bool SkOpCoincidence::addOrOverlap(SkOpSegment* coinSeg, SkOpSegment* oppSeg,
                 : overlap->oppPtTEnd()->fT < test->oppPtTEnd()->fT) {
             overlap->setOppPtTEnd(test->oppPtTEnd());
         }
-        if (!fHead || !this->release(fHead, test)) {
-            SkAssertResult(this->release(fTop, test));
+        if (!fHead || !this->release(&fHead, test)) {
+            SkAssertResult(this->release(&fTop, test));
         }
     }
     const SkOpPtT* cs = coinSeg->existing(coinTs, oppSeg);
@@ -1157,57 +1157,54 @@ bool SkOpCoincidence::apply(DEBUG_COIN_DECLARE_ONLY_PARAMS()) {
 }
 
 // Please keep this in sync with debugRelease()
-bool SkOpCoincidence::release(SkCoincidentSpans* coin, SkCoincidentSpans* remove)  {
-    SkCoincidentSpans* head = coin;
+bool SkOpCoincidence::release(SkCoincidentSpans** headPtr, SkCoincidentSpans* remove)  {
+    SkASSERT(headPtr == &fHead || headPtr == &fTop);
+    SkCoincidentSpans* coin = *headPtr;
     SkCoincidentSpans* prev = nullptr;
     SkCoincidentSpans* next;
-    do {
+    while (coin) {
         next = coin->next();
         if (coin == remove) {
             if (prev) {
                 prev->setNext(next);
-            } else if (head == fHead) {
-                fHead = next;
             } else {
-                fTop = next;
+                *headPtr = next;
             }
             break;
         }
         prev = coin;
-    } while ((coin = next));
+        coin = next;
+    }
     return coin != nullptr;
 }
 
-void SkOpCoincidence::releaseDeleted(SkCoincidentSpans* coin) {
-    if (!coin) {
-        return;
-    }
-    SkCoincidentSpans* head = coin;
+void SkOpCoincidence::releaseDeleted(SkCoincidentSpans** headPtr) {
+    SkASSERT(headPtr == &fHead || headPtr == &fTop);
+    SkCoincidentSpans* coin = *headPtr;
     SkCoincidentSpans* prev = nullptr;
     SkCoincidentSpans* next;
-    do {
+    while (coin) {
         next = coin->next();
         if (coin->coinPtTStart()->deleted()) {
             SkOPASSERT(coin->flipped() ? coin->oppPtTEnd()->deleted() :
                     coin->oppPtTStart()->deleted());
             if (prev) {
                 prev->setNext(next);
-            } else if (head == fHead) {
-                fHead = next;
             } else {
-                fTop = next;
+                *headPtr = next;
             }
         } else {
              SkOPASSERT(coin->flipped() ? !coin->oppPtTEnd()->deleted() :
                     !coin->oppPtTStart()->deleted());
             prev = coin;
         }
-    } while ((coin = next));
+        coin = next;
+    }
 }
 
 void SkOpCoincidence::releaseDeleted() {
-    this->releaseDeleted(fHead);
-    this->releaseDeleted(fTop);
+    this->releaseDeleted(&fHead);
+    this->releaseDeleted(&fTop);
 }
 
 void SkOpCoincidence::restoreHead() {
@@ -1248,7 +1245,7 @@ bool SkOpCoincidence::expand(DEBUG_COIN_DECLARE_ONLY_PARAMS()) {
                 }
                 if (coin->coinPtTStart() == test->coinPtTStart()
                         && coin->oppPtTStart() == test->oppPtTStart()) {
-                    this->release(fHead, test);
+                    this->release(&fHead, test);
                     break;
                 }
             } while ((test = test->next()));
@@ -1297,45 +1294,52 @@ bool SkOpCoincidence::findOverlaps(SkOpCoincidence* overlaps  DEBUG_COIN_DECLARE
 void SkOpCoincidence::fixUp(SkOpPtT* deleted, const SkOpPtT* kept) {
     SkOPASSERT(deleted != kept);
     if (fHead) {
-        this->fixUp(fHead, deleted, kept);
+        this->fixUp(&fHead, deleted, kept);
     }
     if (fTop) {
-        this->fixUp(fTop, deleted, kept);
+        this->fixUp(&fTop, deleted, kept);
     }
 }
 
-void SkOpCoincidence::fixUp(SkCoincidentSpans* coin, SkOpPtT* deleted, const SkOpPtT* kept) {
-    SkCoincidentSpans* head = coin;
-    do {
+void SkOpCoincidence::fixUp(SkCoincidentSpans** headPtr, SkOpPtT* deleted, const SkOpPtT* kept) {
+    SkASSERT(headPtr == &fHead || headPtr == &fTop);
+    SkCoincidentSpans* coin = *headPtr;
+    while (coin) {
+        SkCoincidentSpans* next = coin->next();
         if (coin->coinPtTStart() == deleted) {
             if (coin->coinPtTEnd()->span() == kept->span()) {
-                this->release(head, coin);
+                this->release(headPtr, coin);
+                coin = next;
                 continue;
             }
             coin->setCoinPtTStart(kept);
         }
         if (coin->coinPtTEnd() == deleted) {
             if (coin->coinPtTStart()->span() == kept->span()) {
-                this->release(head, coin);
+                this->release(headPtr, coin);
+                coin = next;
                 continue;
             }
             coin->setCoinPtTEnd(kept);
        }
         if (coin->oppPtTStart() == deleted) {
             if (coin->oppPtTEnd()->span() == kept->span()) {
-                this->release(head, coin);
+                this->release(headPtr, coin);
+                coin = next;
                 continue;
             }
             coin->setOppPtTStart(kept);
         }
         if (coin->oppPtTEnd() == deleted) {
             if (coin->oppPtTStart()->span() == kept->span()) {
-                this->release(head, coin);
+                this->release(headPtr, coin);
+                coin = next;
                 continue;
             }
             coin->setOppPtTEnd(kept);
         }
-    } while ((coin = coin->next()));
+        coin = next;
+    }
 }
 
 // Please keep this in sync with debugMark()
@@ -1386,9 +1390,11 @@ bool SkOpCoincidence::mark(DEBUG_COIN_DECLARE_ONLY_PARAMS()) {
 }
 
 // Please keep in sync with debugMarkCollapsed()
-void SkOpCoincidence::markCollapsed(SkCoincidentSpans* coin, SkOpPtT* test) {
-    SkCoincidentSpans* head = coin;
+void SkOpCoincidence::markCollapsed(SkCoincidentSpans** headPtr, SkOpPtT* test) {
+    SkASSERT(headPtr == &fHead || headPtr == &fTop);
+    SkCoincidentSpans* coin = *headPtr;
     while (coin) {
+        SkCoincidentSpans* next = coin->next();
         if (coin->collapsed(test)) {
             if (zero_or_one(coin->coinPtTStart()->fT) && zero_or_one(coin->coinPtTEnd()->fT)) {
                 coin->coinPtTStartWritable()->segment()->markAllDone();
@@ -1396,16 +1402,16 @@ void SkOpCoincidence::markCollapsed(SkCoincidentSpans* coin, SkOpPtT* test) {
             if (zero_or_one(coin->oppPtTStart()->fT) && zero_or_one(coin->oppPtTEnd()->fT)) {
                 coin->oppPtTStartWritable()->segment()->markAllDone();
             }
-            this->release(head, coin);
+            this->release(headPtr, coin);
         }
-        coin = coin->next();
+        coin = next;
     }
 }
 
 // Please keep in sync with debugMarkCollapsed()
 void SkOpCoincidence::markCollapsed(SkOpPtT* test) {
-    markCollapsed(fHead, test);
-    markCollapsed(fTop, test);
+    markCollapsed(&fHead, test);
+    markCollapsed(&fTop, test);
 }
 
 bool SkOpCoincidence::Ordered(const SkOpSegment* coinSeg, const SkOpSegment* oppSeg) {
@@ -1439,18 +1445,16 @@ bool SkOpCoincidence::overlap(const SkOpPtT* coin1s, const SkOpPtT* coin1e,
     return *overS < *overE;
 }
 
-// Commented-out lines keep this in sync with debugRelease()
 void SkOpCoincidence::release(const SkOpSegment* deleted) {
     SkCoincidentSpans* coin = fHead;
-    if (!coin) {
-        return;
-    }
-    do {
+    while (coin) {
+        SkCoincidentSpans* next = coin->next();
         if (coin->coinPtTStart()->segment() == deleted
                 || coin->coinPtTEnd()->segment() == deleted
                 || coin->oppPtTStart()->segment() == deleted
                 || coin->oppPtTEnd()->segment() == deleted) {
-            this->release(fHead, coin);
+            this->release(&fHead, coin);
         }
-    } while ((coin = coin->next()));
+        coin = next;
+    }
 }
diff --git a/src/pathops/SkOpCoincidence.h b/src/pathops/SkOpCoincidence.h
index cf8bb18d78..3f4b26371d 100644
--- a/src/pathops/SkOpCoincidence.h
+++ b/src/pathops/SkOpCoincidence.h
@@ -282,13 +282,13 @@ private:
     void debugAddEndMovedSpans(SkPathOpsDebug::GlitchLog* ,
                                const SkOpPtT* ptT) const;
 #endif
-    void fixUp(SkCoincidentSpans* coin, SkOpPtT* deleted, const SkOpPtT* kept);
-    void markCollapsed(SkCoincidentSpans* head, SkOpPtT* test);
+    void fixUp(SkCoincidentSpans** headPtr, SkOpPtT* deleted, const SkOpPtT* kept);
+    void markCollapsed(SkCoincidentSpans** headPtr, SkOpPtT* test);
     bool overlap(const SkOpPtT* coinStart1, const SkOpPtT* coinEnd1,
                  const SkOpPtT* coinStart2, const SkOpPtT* coinEnd2,
                  double* overS, double* overE) const;
-    bool release(SkCoincidentSpans* coin, SkCoincidentSpans* );
-    void releaseDeleted(SkCoincidentSpans* );
+    bool release(SkCoincidentSpans** headPtr, SkCoincidentSpans* );
+    void releaseDeleted(SkCoincidentSpans** headPtr);
     void restoreHead();
     // return coinPtT->segment()->t mapped from overS->fT <= t <= overE->fT
     static double TRange(const SkOpPtT* overS, double t, const SkOpSegment* coinPtT
-- 
2.43.0

