Fix Undefined Behavior by replacing illegal overlap test with memmove()#924
Open
imaami wants to merge 5 commits intoDaveGamble:masterfrom
Open
Fix Undefined Behavior by replacing illegal overlap test with memmove()#924imaami wants to merge 5 commits intoDaveGamble:masterfrom
imaami wants to merge 5 commits intoDaveGamble:masterfrom
Conversation
…gs_overlap" This reverts commit 078c4e6.
This reverts commit 4f4d7f7.
This reverts commit b47edc4.
According to the C standard it is Undefined Behavior to arithmetically compare addresses that don't point to within the same object. There is simply no way to implement this sort of check legally. The correct way to avoid problems from overlapping strings is to replace strcpy() with memmove().
Minor optimizations to cJSON_SetValueString() program flow: - NULL check the input first before going into more detail. - Check the type flags with one bitwise AND instead of two. - Remove a redundant second object->valuestring NULL check.
Author
|
@DaveGamble are you busy with life these days? It's understandable. But it would be nice to see cJSON improve, too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I pointed this out in #919, sorry for taking so long to make a PR. But here it is. No need to check for string overlap–it's simply not legal in C, and
memmove()exists to guarantee that overlapping copies don't cause unforeseen problems.