[PATCH] D57023: [MsgPack] New MsgPackDocument class
Scott Linder via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 24 13:01:46 PDT 2020
scott.linder added inline comments.
================
Comment at: llvm/trunk/include/llvm/BinaryFormat/MsgPackDocument.h:123-132
+ /// Get an ArrayDocNode for an array node. If Convert, convert the node to an
+ /// array node if necessary.
+ ArrayDocNode &getArray(bool Convert = false) {
+ if (getKind() != Type::Array) {
+ assert(Convert);
+ convertToArray();
+ }
----------------
dblaikie wrote:
> Excuse the delayed review - found this while investigating other issues. I believe this code is invalid & is an aliasing violation (taking a pointer to ArrayDocNode (similarly with MapDocNode) when it's not pointing to an object of that type).
>
> Can you please fix this code to not do that?
Is the issue that in the implementations:
```
void DocNode::convertToArray() { *this = getDocument()->getArrayNode(); }
```
and
```
ArrayDocNode getArrayNode() {
auto N = DocNode(&KindAndDocs[size_t(Type::Array)]);
Arrays.push_back(std::unique_ptr<DocNode::ArrayTy>(new DocNode::ArrayTy));
N.Array = Arrays.back().get();
return N.getArray();
}
```
As even though the object pointed to by `this` is destructed and a new ArrayDocNode is copy-constructed over it, *that* `ArrayDocNode` is not actually ever constructed. A `DocNode` is just "type punned" into an `ArrayDocNode`.
Would changing the definition to:
```
ArrayDocNode getArrayNode() {
auto N = ArrayDocNode(&KindAndDocs[size_t(Type::Array)]);
Arrays.push_back(std::unique_ptr<DocNode::ArrayTy>(new DocNode::ArrayTy));
N.Array = Arrays.back().get();
return N;
}
```
And the analogous change for MapDocNode avoid the UB?
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57023/new/
https://reviews.llvm.org/D57023
More information about the llvm-commits
mailing list