Repository navigation
Fix TypeBuilder canonicalization - #3578
Conversation
When types or heap types were used multiple times in a TypeBuilder instance, it was possible for the canonicalization algorithm to canonicalize a parent type before canonicalizing all of its component child types, leaking the temporary types into globally interned types. This bug led to incorrect canonicalization results and use-after free bugs. The cause of the bug was that types were canonicalized in the reverse of the order that they were visited in, but children were visited after the first occurrence of their parents, not necessarily after the last occurrence of their parents. One fix could have been to remove the logic that prevented types from being visited multiple times so that children would always be visited after their parents. That simple fix, however, would not scale gracefully to handle recursive types because it would require some way to detect recursions without accidentally reintroducing these bugs. This PR implements a more robust solution: topologically sorting the traversed types to ensure that children are canonicalized before their parents. This solution will be trivial to adapt for recursive types because recursive types are trivial to detect from the reachability graph used to perform the topological sort.
| auto childrenIt = ancestors.find(i); | ||
| if (childrenIt != ancestors.end()) { | ||
| for (auto child : childrenIt->second) { | ||
| visit(child); |
There was a problem hiding this comment.
I'm missing something. ancestors is defined as
// Maps Type and HeapType IDs to the IDs of their ancestor Types and HeapTypes
// in the type graph. Only considers compound Types and HeapTypes.From this description I expect ancestors[id] to be a list of the ancestors of id. But the loop here instead calls those items "children" - ?
There was a problem hiding this comment.
Oh yeah, I got myself confused about the correct direction of the relation when I was writing this and I guess I forgot to update this code. I'll fix this. ancestors and your expectation are correct.
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
|
|
||
| // Collect the items to be sorted, including all the result HeapTypes and any | ||
| // type that participates in the ancestor relation. | ||
| std::set<TypeID> allIDs; |
There was a problem hiding this comment.
| std::set<TypeID> allIDs; | |
| std::unordered_set<TypeID> allIDs; |
I doubt it matters for speed, but it may be clearer to read, as it indicates there is no sorting order in this data structure?
There was a problem hiding this comment.
I intentionally used a set here so that the result of the topological would be deterministic. I'll add a comment.
There was a problem hiding this comment.
Ah, then I missed something.
Do you mean it keeps the sort at each topological depth deterministic?
There was a problem hiding this comment.
I meant for this std::set to keep the top-level driving of the sort deterministic and for the std::set in ancestors to keep each recursive step of the sort deterministic. But now that I think about it, the TypeIDs themselves aren't deterministic, so using a std::set here is not sufficient to make the sort deterministic. Furthermore, it shouldn't matter whether this sort is deterministic; that shouldn't be visible to the user or affect anything else. I'll switch to using std::unordered_set here and in ancestors.
When types or heap types were used multiple times in a TypeBuilder instance, it
was possible for the canonicalization algorithm to canonicalize a parent type
before canonicalizing all of its component child types, leaking the temporary
types into globally interned types. This bug led to incorrect canonicalization
results and use-after free bugs.
The cause of the bug was that types were canonicalized in the reverse of the
order that they were visited in, but children were visited after the first
occurrence of their parents, not necessarily after the last occurrence of their
parents. One fix could have been to remove the logic that prevented types from
being visited multiple times so that children would always be visited after
their parents. That simple fix, however, would not scale gracefully to handle
recursive types because it would require some way to detect recursions without
accidentally reintroducing these bugs.
This PR implements a more robust solution: topologically sorting the traversed
types to ensure that children are canonicalized before their parents. This
solution will be trivial to adapt for recursive types because recursive types
are trivial to detect from the reachability graph used to perform the
topological sort.
This supersedes the incomplete fix in #3571.