[PATCH] D72529: [SCCIterator] Fix use-after-free

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

Independently, we came across this same problem, and I proposed a patch at D72469 <https://reviews.llvm.org/D72469> (which has been committed) that is essentially the simple fix that is described above:

> `unsigned &OldRef = nodeVisitNumbers[Old];`
>  `unsigned &NewRef = nodeVisitNumbers[New];`
>  `unsigned OldVal = OldRef;`
>  `NewRef = OldVal;`

Regarding the point raised here about this simpler solution:

> An obvious fix is just to manually sequence the operations. However,
>  I would be concerned that this issue could be re-introduced later
>  by somebody assuming that removing the intermediate variable is NFC.

First, I'm certainly not opposed to replacing the simpler solution with the iterator interface approach.

Second, I also was concerned that it may appear that someone may mistakenly think that removing the temporary would be safe, so in my change, I included a comment explaining the reason for the temporary.  For reference here, the committed change (along with the comment) is:

  // Do the assignment in two steps, in case 'New' is not yet in the map, and
  // inserting it causes the map to grow.
  auto tempVal = nodeVisitNumbers[Old];
  nodeVisitNumbers[New] = tempVal;
  nodeVisitNumbers.erase(Old);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72529/new/

https://reviews.llvm.org/D72529