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

Fix (Sub-)Value assignment #1503

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Conversation

ylavic
Copy link
Contributor

@ylavic ylavic commented Apr 28, 2019

Extracted from PR #1485, should probably live on its own...

The issue is, from valuetest.cpp::MergeDuplicateKey():

        for (Value::MemberIterator itr = v.MemberBegin(); itr != v.MemberEnd(); ++itr) {
            if (itr->value.Size() == 1)
                itr->value = itr->value[0];
            ...
        }

Without the fix, in current GenericValue& operator=(GenericValue&), itr->value.~GenericValue() gets called before itr->value.RawAssign(itr->value[0]) and thus itr->value[0] may be used after free (with a kneedFree allocator).

PS: the VC2013 compilation fix for sortkeys.cpp is also included here to pass ci.

@coveralls
Copy link

coveralls commented Apr 28, 2019

Coverage Status

Coverage increased (+0.0001%) to 99.729% when pulling 117276c on ylavic:sub_value_assignment into 49aa0fc on Tencent:master.

@miloyip
Copy link
Collaborator

miloyip commented Oct 10, 2019

I suggest adding an unit test for testing this case.

When rhs is a sub-Value of *this, destroying *this also destroys/frees
rhs, thus the following RawAssign(rhs) crashes.

Address this by saving/moving rhs to a temporary first, which clears rhs
and avoids its destruction with *this.

The crash can be reproduced in test Value.MergeDuplicateKey by using the
CrtAllocator instead of the default Document's MemoryPoolAllocator.
@ylavic
Copy link
Contributor Author

ylavic commented Mar 15, 2021

I suggest adding an unit test for testing this case.

Done.

The allocator cannot be destroyed before the Document, otherwise the
Value destructor double frees.
@miloyip miloyip merged commit 7d801bb into Tencent:master Apr 7, 2021
@ylavic ylavic deleted the sub_value_assignment branch April 7, 2021 10:04
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

Successfully merging this pull request may close these issues.

None yet

3 participants