[PATCH] D72529: [SCCIterator] Fix use-after-free
Warren Ristow via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 14 23:21:51 PST 2020
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
More information about the llvm-commits
mailing list