Repository navigation
Fix data loss when mixing line and block comments - #233
Open
jeff-grimes-pplx wants to merge 1 commit into
Open
jeff-grimes-pplx wants to merge 1 commit into
jeff-grimes-pplx wants to merge 1 commit into
Conversation
json_parse_string_with_comments() stripped comments in two independent
passes: first every "/* ... */", then every "// ...". Each pass only
tracked string literals, not the other comment style, so text inside a
comment of one style was interpreted as code by the other pass:
* A line comment containing "/*" (e.g. "// matches src/*.c") started a
block comment in the first pass that ran up to the next "*/" later in
the document, silently deleting the JSON in between, e.g.
{"a":1, // src/*.c\n "b":2, /* x */\n "c":3} parsed as {"a":1,"c":3}.
* A lone double quote in a line comment (e.g. "// 5\" screen") flipped
the first pass's in-string state, so later block comments were left
in place and valid input failed to parse.
Strip both comment styles in a single left-to-right pass so that whichever
construct (string, block comment or line comment) starts first owns the
text that follows it.
This branch has not been deployed
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.
Problem
json_parse_string_with_comments()(and thereforejson_parse_file_with_comments()) silently drops data or fails to parse valid commented JSON when a line comment contains/*or a lone".A line comment with an unmatched quote (
// 5" screen) followed by a block comment makes the whole document fail to parse (NULL).Root Cause
Comments are stripped by two independent passes:
Each pass only knows about string literals and its own comment style. During the
/* */pass, the/*inside// matches src/*.cis treated as the start of a block comment, which then runs until the*/of the next comment, blanking out the"b": 2,member in between. Likewise, a"inside a line comment flips the block pass'sin_stringstate, so the following/* px */is never removed and the parser sees garbage.Fix
Replace the two passes with a single left-to-right pass that tracks string literals (including escapes),
/* ... */comments and// ...comments at the same time, so whichever construct opens first wins — exactly as a C/JS lexer would. Behaviour for every input that previously worked is unchanged: comment bodies are still overwritten with spaces, the newline that ends a line comment is preserved, and an unterminated block comment still blanks to the end of the input.Minimal Reproducer
Compiling and running the snippet above under ASan/UBSan:
Before:
[FAIL] line comment containing '/*' must not swallow data(actual{"a":1,"c":3}) and[FAIL] line comment containing a lone '"' must not hide a block comment(actualNULL). After: all cases pass.Verification
test_suite_2_mixed_comments()intests.c(4 cases) — fails onmaster(Tests failed: 4), passes with the fix.make test,make testcpp,make test_hash_collisions:Tests failed: 0, Tests passed: 369each.