Fix ResumableParser losing tokens before a feed-boundary suspension

ext/json/ext/parser/parser.c

@@ -768,13 +768,18 @@ static const rb_data_type_t JSON_ParserConfig_type;
 
 const char *COMMENT_DEPRECATION_MESSAGE = "Encountered comment in JSON. This will raise an error in json 3.0 unless enabled via `allow_comments: true`";
 NOINLINE(static) void
-json_eat_comments(JSON_ParserState *state, JSON_ParserConfig *config)
+json_eat_comments(JSON_ParserState *state, JSON_ParserConfig *config, const char *resume_pos)
 {
     if (config->on_comment == JSON_RAISE) {
         raise_syntax_error("unexpected token %s", state);
     }
 
     const char *start = state->cursor;
+    // An incomplete comment suspends a resumable parse by rewinding the cursor
+    // and throwing. Callers that already consumed a token not yet committed to
+    // the frame stack pass resume_pos so the rewind re-reads that token too.
+    // Non-resumable error positions keep pointing at the comment either way.
+    const char *rewind_pos = (state->parser && resume_pos) ? resume_pos : start;
     state->cursor++;
 
     switch (peek(state)) {
@@ -786,7 +791,7 @@ json_eat_comments(JSON_ParserState *state, JSON_ParserConfig *config)
                 // the comment unterminated instead of consuming to end as a one-shot
                 // parse would.
                 if (state->parser) {
-                    raise_eos_error_at("unterminated comment, expected end of line", state, start);
+                    raise_eos_error_at("unterminated comment, expected end of line", state, rewind_pos);
                 }
                 state->cursor = state->end;
             } else {
@@ -800,7 +805,7 @@ json_eat_comments(JSON_ParserState *state, JSON_ParserConfig *config)
             while (true) {
                 const char *next_match = memchr(state->cursor, '*', state->end - state->cursor);
                 if (!next_match) {
-                    raise_eos_error_at("unterminated comment, expected closing '*/'", state, start);
+                    raise_eos_error_at("unterminated comment, expected closing '*/'", state, rewind_pos);
                 }
 
                 state->cursor = next_match + 1;
@@ -812,7 +817,7 @@ json_eat_comments(JSON_ParserState *state, JSON_ParserConfig *config)
             break;
         }
         default:
-            raise_parse_error_at("unexpected token %s", state, start, eos(state));
+            raise_parse_error_at("unexpected token %s", state, eos(state) ? rewind_pos : start, eos(state));
             break;
     }
 
@@ -823,7 +828,7 @@ json_eat_comments(JSON_ParserState *state, JSON_ParserConfig *config)
 }
 
 ALWAYS_INLINE(static) void
-json_eat_whitespace(JSON_ParserState *state, JSON_ParserConfig *config, bool include_comments)
+json_eat_whitespace_resume_at(JSON_ParserState *state, JSON_ParserConfig *config, bool include_comments, const char *resume_pos)
 {
     while (true) {
         switch (peek(state)) {
@@ -858,7 +863,7 @@ json_eat_whitespace(JSON_ParserState *state, JSON_ParserConfig *config, bool inc
                     return;
                 }
 
-                json_eat_comments(state, config);
+                json_eat_comments(state, config, resume_pos);
                 break;
 
             default:
@@ -867,6 +872,12 @@ json_eat_whitespace(JSON_ParserState *state, JSON_ParserConfig *config, bool inc
     }
 }
 
+ALWAYS_INLINE(static) void
+json_eat_whitespace(JSON_ParserState *state, JSON_ParserConfig *config, bool include_comments)
+{
+    json_eat_whitespace_resume_at(state, config, include_comments, NULL);
+}
+
 static inline VALUE build_string(const char *start, const char *end, bool intern, bool symbolize)
 {
     if (symbolize) {
@@ -1580,6 +1591,13 @@ ALWAYS_INLINE(static) bool json_parse_any(JSON_ParserState *state, JSON_ParserCo
     JSON_PHASE_VALUE: {
         json_eat_whitespace(state, config, true);
 
+        // A trailing comma lands us here expecting an element but finding the
+        // closing bracket; hand off to ARRAY_COMMA to close. An empty array
+        // closes inline at '[', so this position is only reached after a ','.
+        if (config->allow_trailing_comma && frame->type == JSON_FRAME_ARRAY && peek(state) == ']') {
+            goto JSON_PHASE_ARRAY_COMMA;
+        }
+
         VALUE value;
         const char *value_start = state->cursor;
 
@@ -1675,7 +1693,9 @@ ALWAYS_INLINE(static) bool json_parse_any(JSON_ParserState *state, JSON_ParserCo
 
             case '[': {
                 state->cursor++;
-                json_eat_whitespace(state, config, true);
+                // The '[' is consumed but its frame is only pushed below, so a
+                // comment suspending here must resume from the bracket.
+                json_eat_whitespace_resume_at(state, config, true, value_start);
 
                 const char next = peek(state);
                 if (next == ']') {
@@ -1704,7 +1724,8 @@ ALWAYS_INLINE(static) bool json_parse_any(JSON_ParserState *state, JSON_ParserCo
 
             case '{': {
                 state->cursor++;
-                json_eat_whitespace(state, config, true);
+                // Same as '[': the frame is only pushed below.
+                json_eat_whitespace_resume_at(state, config, true, value_start);
 
                 if (peek(state) == '}') {
                     state->cursor++;
@@ -1762,6 +1783,13 @@ ALWAYS_INLINE(static) bool json_parse_any(JSON_ParserState *state, JSON_ParserCo
 
         json_eat_whitespace(state, config, true);
 
+        // A trailing comma lands us here expecting a key but finding the closing
+        // brace; hand off to OBJECT_COMMA to close. An empty object closes inline
+        // at '{', so this position is only reached after a ','.
+        if (config->allow_trailing_comma && peek(state) == '}') {
+            goto JSON_PHASE_OBJECT_COMMA;
+        }
+
         const char *start = state->cursor;
 
         if (RB_LIKELY(peek(state) == '"')) {
@@ -1824,13 +1852,10 @@ ALWAYS_INLINE(static) bool json_parse_any(JSON_ParserState *state, JSON_ParserCo
 
         if (RB_LIKELY(next_char == ',')) {
             state->cursor++;
-            if (config->allow_trailing_comma) {
-                json_eat_whitespace(state, config, true);
-                if (peek(state) == ']') {
-                    // Trailing comma: stay in COMMA to close on the next iteration.
-                    goto JSON_PHASE_ARRAY_COMMA;
-                }
-            }
+            // Commit the phase before eating the whitespace that follows: an
+            // incomplete comment there would suspend the parse, and a phase not
+            // yet advanced past the ',' would drop it on resume. A trailing comma
+            // is recognized in JSON_PHASE_VALUE once the ']' is in the buffer.
             frame->phase = JSON_PHASE_VALUE;
             goto JSON_PHASE_VALUE;
         } else if (next_char == ']') {
@@ -1869,15 +1894,10 @@ ALWAYS_INLINE(static) bool json_parse_any(JSON_ParserState *state, JSON_ParserCo
 
         if (RB_LIKELY(next_char == ',')) {
             state->cursor++;
-            json_eat_whitespace(state, config, true);
-
-            if (config->allow_trailing_comma) {
-                if (peek(state) == '}') {
-                    // Trailing comma: stay in COMMA to close on the next iteration.
-                    goto JSON_PHASE_OBJECT_COMMA;
-                }
-            }
-
+            // Commit the phase before eating the whitespace that follows: an
+            // incomplete comment there would suspend the parse, and a phase not
+            // yet advanced past the ',' would drop it on resume. A trailing comma
+            // is recognized in JSON_PHASE_OBJECT_KEY once the '}' is in the buffer.
             frame->phase = JSON_PHASE_OBJECT_KEY;
             goto JSON_PHASE_OBJECT_KEY;
         } else if (next_char == '}') {

test/json/resumable_parser_test.rb

@@ -241,6 +241,105 @@ def test_block_comment_spanning_feed_boundary_is_not_terminated_early
     assert_equal [[1], [3]], values
   end
 
+  def test_trailing_comma_split_across_feed_boundary
+    # With allow_trailing_comma the closing bracket may arrive in a later chunk
+    # than the comma; consuming the comma must not lose the ability to close.
+    parser = new_parser(allow_trailing_comma: true)
+    parser << '[1,'
+    refute parser.parse
+    parser << ']'
+    assert parser.parse
+    assert_equal [1], parser.value
+
+    parser = new_parser(allow_trailing_comma: true)
+    parser << '{"a":1,'
+    refute parser.parse
+    parser << '}'
+    assert parser.parse
+    assert_equal({ "a" => 1 }, parser.value)
+
+    # The boundary can also fall after an inner comma, then after the outer one.
+    parser = new_parser(allow_trailing_comma: true)
+    parser << '[[1,'
+    refute parser.parse
+    parser << '],]'
+    assert parser.parse
+    assert_equal [[1]], parser.value
+  end
+
+  def test_trailing_comma_byte_by_byte
+    parser = new_parser(allow_trailing_comma: true)
+    '[1, 2, ]'.each_char { |c| parser << c; parser.parse }
+    assert_equal [1, 2], parser.value
+
+    parser = new_parser(allow_trailing_comma: true)
+    '{ "a": 1, }'.each_char { |c| parser << c; parser.parse }
+    assert_equal({ "a" => 1 }, parser.value)
+  end
+
+  def test_comment_after_comma_split_across_feed_boundary
+    # A comment right after a ',' straddling a feed boundary must not drop the
+    # comma: the value/key it separates must still be parsed on resume.
+    # The array case needs allow_trailing_comma: without it the array comma path
+    # commits its phase before eating the comment, so only the trailing-comma
+    # path exercises the eat-before-commit bug (the object path always did).
+    parser = new_parser(allow_comments: true, allow_trailing_comma: true)
+    parser << '[1,/*'
+    refute parser.parse
+    parser << '*/2]'
+    assert parser.parse
+    assert_equal [1, 2], parser.value
+
+    parser = new_parser(allow_comments: true)
+    parser << '{"a":1,/*'
+    refute parser.parse
+    parser << '*/"b":2}'
+    assert parser.parse
+    assert_equal({ "a" => 1, "b" => 2 }, parser.value)
+  end
+
+  def test_comment_after_container_open_split_across_feed_boundary
+    # A comment right after '[' or '{' straddling a feed boundary must not drop
+    # the opening token: it is consumed before its frame is pushed, so the
+    # suspension must resume from the bracket, not from inside the comment.
+    parser = new_parser(allow_comments: true)
+    parser << '[/*'
+    refute parser.parse
+    parser << '*/1]'
+    assert parser.parse
+    assert_equal [1], parser.value
+
+    parser = new_parser(allow_comments: true)
+    parser << '{/*'
+    refute parser.parse
+    parser << '*/"a":1}'
+    assert parser.parse
+    assert_equal({ "a" => 1 }, parser.value)
+
+    parser = new_parser(allow_comments: true)
+    parser << '[ /*'
+    refute parser.parse
+    parser << '*/ ]'
+    assert parser.parse
+    assert_equal [], parser.value
+
+    # The boundary can even split the comment marker itself.
+    parser = new_parser(allow_comments: true)
+    parser << '[/'
+    refute parser.parse
+    parser << '**/1]'
+    assert parser.parse
+    assert_equal [1], parser.value
+
+    # Line comments suspend the same way when their newline hasn't arrived.
+    parser = new_parser(allow_comments: true)
+    parser << '[//'
+    refute parser.parse
+    parser << "x\n1]"
+    assert parser.parse
+    assert_equal [1], parser.value
+  end
+
   def test_rest
     @parser << '[1, 2, 3, "unterminated string'
     refute @parser.parse