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

Avoid calling memcpy with NULL pointers #305

Merged
merged 1 commit into from
Apr 17, 2015

Conversation

pah
Copy link
Contributor

@pah pah commented Apr 15, 2015

According to the C/C++ standards, calling memcpy(NULL, NULL, 0) is undefined behaviour. Recent GCC versions may rely on this by optimizing NULL pointer checks more aggressively, see [1].

This patch tries to avoid calling std::memcpy with zero elements. As a side effect, explicitly return NULL when requesting an empty block from MemoryPoolAllocator::Malloc.

This may be related to #301. Opening the pull-request as requested by @miloyip.

[1] https://gcc.gnu.org/gcc-4.9/porting_to.html

@miloyip
Copy link
Collaborator

miloyip commented Apr 16, 2015

I think adding a branch in SetObjectRaw() and SetArrayRaw() is not a good idea.
They are only called by EndObject() and EndArray() respectively. And the parameter should never been NULL. Adding assertions should be enough.

@pah
Copy link
Contributor Author

pah commented Apr 16, 2015

Are you concerned about the performance? I have not seen any difference caused by this change.
But in case of empty arrays/objects (count == 0, why wouldn't the pointers be NULL, too?

@miloyip
Copy link
Collaborator

miloyip commented Apr 16, 2015

If the objective is to prevent calling memcpy() when count == 0, I think it should be:

if (count) {
    data_.a.elements = (GenericValue*)allocator.Malloc(count * sizeof(GenericValue));
    std::memcpy(data_.a.elements, values, count * sizeof(GenericValue));
}
else
    data_.a.elements = 0;
data_.a.size = data_.a.capacity = count;

values is always non-null.

@pah
Copy link
Contributor Author

pah commented Apr 16, 2015

Oh, you're right. values comes from the stack. Will adjust the PR tonight.

@pah pah force-pushed the fix/strict-memcpy branch from d9b44b9 to ee7522a Compare April 16, 2015 19:03
@pah
Copy link
Contributor Author

pah commented Apr 16, 2015

PR updated, see 0c5c153.

According to the C/C++ standards, calling `memcpy(NULL, NULL, 0)` is
undefined behaviour. Recent GCC versions may rely on this by optimizing
NULL pointer checks more aggressively, see [1].

This patch tries to avoid calling std::memcpy with zero elements.
As a side effect, explicitly return NULL when requesting an empty block
from MemoryPoolAllocator::Malloc.

This may be related to Tencent#301.

[1] https://gcc.gnu.org/gcc-4.9/porting_to.html
@pah pah force-pushed the fix/strict-memcpy branch from ee7522a to 0c5c153 Compare April 16, 2015 19:05
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 99.96% when pulling 0c5c153 on pah:fix/strict-memcpy into 30ace6f on miloyip:master.

@coveralls
Copy link

coveralls commented Apr 16, 2015

Coverage Status

Coverage decreased (-0.05%) to 99.952% when pulling 0c5c153 on pah:fix/strict-memcpy into 30ace6f on miloyip:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 99.96% when pulling 0c5c153 on pah:fix/strict-memcpy into 30ace6f on miloyip:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 99.96% when pulling 0c5c153 on pah:fix/strict-memcpy into 30ace6f on miloyip:master.

miloyip added a commit that referenced this pull request Apr 17, 2015
Avoid calling memcpy with NULL pointers
@miloyip miloyip merged commit 4cd14b7 into Tencent:master Apr 17, 2015
@pah pah deleted the fix/strict-memcpy branch April 17, 2015 07:03
@miloyip
Copy link
Collaborator

miloyip commented Apr 17, 2015

I added a test case and also standardize CrtAllocator::Malloc() in 0e8bbe5.

@pah
Copy link
Contributor Author

pah commented Apr 17, 2015

Thanks! 👍

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.

3 participants