[PATCH] D57023: [MsgPack] New MsgPackDocument class

[Scott Linder via Phabricator via llvm-commits]

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