Conversation
validate_protocol computes the character just before the protocol with:
char prev_char = data[-((ptrdiff_t)rewind) - len - 1];
len is a size_t, so the usual arithmetic conversions make the whole index a
size_t: instead of a small negative offset the expression evaluates to a huge
unsigned value, and adding it to data wraps the pointer. That is undefined
behaviour, and UBSan builds abort on it:
extensions/autolink.c:315:20: runtime error: addition of unsigned offset
to 0x6080000009d5 overflowed to 0x6080000009c6
The address that is finally computed is the intended one and lies inside the
text buffer (the guard above guarantees rewind + len + 1 <= max_rewind), so the
only problem is the wrapping arithmetic itself. It is still undefined
behaviour, and it makes every UBSan build abort on crafted input even when the
resulting parse is correct.
Cast the complete offset to ptrdiff_t so the index stays signed and the pointer
arithmetic cannot wrap. The byte that is read is unchanged, so there is no
change in observable behaviour.
Found with UBSan using the upstream test/cmark-fuzz.c driver.
Fixes github#393
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
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.
validate_protocol()reads the character immediately before a matched protocol:lenis asize_t, so the usual arithmetic conversions promote the whole index tosize_t. Instead of the intended small negative offset the expression evaluates to ahuge unsigned value, and adding it to
datawraps the pointer. UBSan aborts on it:The address that is finally computed is the intended one and lies inside the text
buffer: the guard right above guarantees
rewind + len + 1 <= max_rewind. The problemis the wrapping arithmetic itself, which is undefined behaviour and makes every UBSan
build abort on crafted input even though the parse is correct.
The fix keeps the index signed:
The byte that is read is unchanged, so there is no change in observable behaviour.
Reproducing
Found with UBSan using the upstream
test/cmark-fuzz.cdriver(
-fsanitize=undefined,address -fno-sanitize-recover=all). The two inputs attached tothe issue reach the same line through different entry points (
validate_protocoldirectly and
postprocess_text); with the 8-byte fuzz config header stripped they alsoreproduce through the CLI:
Both runs are clean with the fix applied,
cteststill passes 13/13, and the renderedHTML for the two inputs is byte-identical before and after.
Fixes #393