Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing a Document by-value to a function taking a Value is broken in c++11 #387

Closed
lukeworthtkbt opened this issue Jul 16, 2015 · 5 comments

Comments

@lukeworthtkbt
Copy link

If we have

Document TransformValue(Value v);

This works (as far as I can tell):

Document d;
d.Parse(json);
Document transformed = TransformValue(std::move(d));

But this causes really confusing problems:

Document ParseDocument(std::string json) {
   Document d;
   d.Parse(json);
   return d;
}

Document transformed = TransformValue(ParseDocument(json));

I think it is because the temporary value returned by ParseDocument gets move-constructed into the argument to TransformValue, but the move is incomplete because it retains the allocator, so when that temporary is deleted, weird stuff starts happening to v inside TransformValue.

@pah
Copy link
Contributor

pah commented Jul 16, 2015

I'd say, this is the expected behaviour. Moving from a Document to a Value obviously only moves the Value part of the Document. Where should the allocator etc. be moved to? Consequently, it will stay in the original Document.

In your expression, the (temporary) allocator gets destroyed after the full expression; The transformed Document now contains the allocator of the (internal) Document, returned by TransformValue. Obviously, this allocator doesn't match the one used to allocate the contents of the incoming Value, which had been allocated by the temporary Document, returned by ParseDocument. So you end up with transformed containing references to memory that has been allocated by different allocators, one of which frees its memory right away.

The same happens in your first code snippet. This will likely break as well, depending on the details of TransformValue.

Long story short: Never mix values from different (even implicit) allocators across Document objects. Either pass the entire Document or at least pass the/an owning allocator as a separate parameter. You could for instance define two overloads (untested):

Document TransformValue(Value v, Document::AllocatorType& alloc) {
  Document d(&alloc);
  v.Swap(d); // "move" v to d
  // original body of the function
  return d;
}
inline Document TransformValue(Document d) {
  return TransformValue(std::move(d), d.GetAllocator());
} 

hth,
Philipp

Edit: Edited to add missing std::move, thanks @lukeworthtkbt.

@lukeworthtkbt
Copy link
Author

Thanks for your response. The problem is that Document is a subclass of Value, which suggests to the user that you can pass a Document where a Value is expected, which leads to using temporary variables like I did in my first example.
Semantically, it seems to me that Document has-a Value and an Allocator, rather than Document is-a Value with an Allocator. I am not the first to suggest this change: #380 (comment)

A simple way to improve the situation would be to put an assertion into GenericValue(GenericValue&&) requiring that the argument is not a GenericDocument (somehow). You need to be able to do this even in @pah's solution above for (what should be) TransformValue(std::move(d), d.GetAllocator()).

pah added a commit to pah/rapidjson-upstream that referenced this issue Jul 17, 2015
As reported in Tencent#387, silently moving a `GenericDocument` to a
`GenericValue` can lead to object slicing and premature deletion of
the owning allocator of the (surviving) `GenericValue`.

To reduce this risk, prohibit move construction of a `GenericValue`
from a `GenericDocument`.
@pah
Copy link
Contributor

pah commented Jul 17, 2015

I agree that with C++11 moves, there additional gotchas in the API. It might be a good idea to explicitly prohibit moving from a Document to a Value, as the document's allocator will likely be destroyed too early. I've pushed a possible reduction of this risk to my fork.

But the biggest issue here is the mixture of Values allocated through different allocators (i.e. from different Documents). You can only do this with the plain CrtAllocator right now. Or you need to somehow use persistent allocators across your internal use of the Document instances.

@mloskot
Copy link
Contributor

mloskot commented Jul 17, 2015

Never mix values from different (even implicit) allocators across Document objects.
Either pass the entire Document or at least pass the/an owning allocator as a separate parameter.

Such clarification on the implicit binding of Value with allocator from parent/owning Document would be helpful in the Tutorial as well as the Internals docs.

@miloyip
Copy link
Collaborator

miloyip commented Jul 22, 2015

Fixed by #391

@miloyip miloyip closed this as completed Jul 22, 2015
danielweck added a commit to readium/readium-lcp-client that referenced this issue Oct 3, 2019
…LCP license is undefined (absent property). The bug was due to the RapidJSON move semantics not copying the allocator in Value objects (only in Document objects) when return a function value. Works fine in function parameters, due to the stacking memory context. Without this fix, the GetType() function returns 16 (or some other weird value) instead of zero for kNullType enum, and isNull() returns false due to internal mismatch in the mask value. https://github.com/Tencent/rapidjson/blob/master/include/rapidjson/document.h#L1954 https://rapidjson.org/md_doc_tutorial.html#TemporaryValues Tencent/rapidjson#387 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants