gumbo: do not use an end tag token whose name has been freed

[jeremy]

gumbo_parse_with_options() crashes when GumboOptions.stop_on_first_error is true and the
parse ends on an unknown end tag inside a fragment context.

This is a C API path. It is not reachable from Ruby. common_options() in
ext/nokogiri/gumbo.c assigns max_attributes, max_errors, max_tree_depth and
parse_noscript_content_as_text, and the fragment path adds fragment_context,
fragment_namespace, fragment_encoding, quirks_mode and
fragment_context_has_form_ancestor. It never assigns stop_on_first_error, and
kGumboDefaultOptions sets it false. rg -n stop_on_first_error ext/ lib/ returns no hits.
So no HTML reaches this through Nokogiri::HTML5. I am reporting and fixing it because the
option is part of the public C interface.

The defect

The tree construction loop frees an unknown end tag's name once it is done with the token, and
sets it to NULL (parser.c:4862-4867):

if (token.type == GUMBO_TOKEN_END_TAG &&
    token.v.end_tag.tag == GUMBO_TAG_UNKNOWN)
{
  gumbo_free(token.v.end_tag.name);
  token.v.end_tag.name = NULL;
}

The loop exit condition can fire on that same iteration:

} while (
  (token.type != GUMBO_TOKEN_EOF || state->_reprocess_current_token)
  && !(options->stop_on_first_error && parser._output->document_error)
);

finish_parsing() then still sees that token as state->_current_token. pop_current_node()
passes tag == GUMBO_TAG_UNKNOWN together with name == NULL into
node_qualified_tagname_is(), whose precondition is the opposite:

assert(tag != GUMBO_TAG_UNKNOWN || name);   // parser.c:603

A fragment context is required. fragment_parser_init pushes an html root onto
_open_elements. In a document parse _open_elements is empty at that point, pop_current_node
returns NULL, and this is never reached.

Two outcomes, depending on how the library was built.

With assertions enabled — which is how Nokogiri builds it — the process aborts on line 603.

With NDEBUG the assertion is gone. If the current node's own tag is also GUMBO_TAG_UNKNOWN
and the namespace matches, node_qualified_tagname_is does not return early at line 607. It
reaches line 611 and calls gumbo_ascii_strcasecmp(element_name, NULL), which dereferences NULL:

AddressSanitizer: SEGV on unknown address 0x000000000000
The signal is caused by a READ memory access.
    #0 gumbo_ascii_strcasecmp        ascii.c:5
    #1 node_qualified_tagname_is     parser.c:611
    #2 pop_current_node              parser.c:1031
    #3 finish_parsing                parser.c:2449
    #4 gumbo_parse_with_options      parser.c:4879

The fix

Such a token cannot name any node, so pop_current_node treats it as no match rather than asking
node_qualified_tagname_is a question that violates its precondition. The node then gets
GUMBO_INSERTION_IMPLICIT_END_TAG, which is what already happens for every other early exit.

Measurements

Host: arm64-darwin, Apple clang. Built directly from gumbo-parser/src with clang -O2, in
both the assertion and the NDEBUG configuration.

input	fragment context	before (assertions)	before (NDEBUG)	after, both
</ta>	template	abort, line 603	ok	ok
</ta>	div	abort, line 603	ok	ok
<custom-el></ta>	div	abort, line 603	SEGV	ok
<custom-el></ta>	template	abort, line 603	SEGV	ok
<foo-bar><baz-qux></ta>	div	abort, line 603	SEGV	ok
<custom-el></zz>	custom-el	abort, line 603	SEGV	ok
</ta>	none (document)	ok	ok	ok

Control: every row above is clean before and after with stop_on_first_error = false.

No behaviour change anywhere else. I built the parser before and after the change and dumped
the full tree for every node — type, tag, namespace, attribute count and parse_flags, since
parse_flags is the only thing this change can affect. Corpus: the 1,792 html5lib
tree-construction cases (192 of them with a fragment context), each parsed with
stop_on_first_error both false and true.

sofe=0   identical  1792
sofe=1   identical  1792

3,584 parses, byte-identical output, no crash on either side.

That corpus contains none of the triggering inputs, so on its own it could not tell a fix from a
no-op. Running the same differential over the cases in the table above shows the instrument does
fire: <custom-el></ta> and the other three unknown-node rows go from exit 139 to exit 0.

Tests

Two tests added to gumbo-parser/test/parser.cc. Without the fix the suite aborts at the first
of them:

[ RUN      ] GumboParserTest.StopOnFirstErrorAfterUnknownEndTag
Assertion failed: (tag != GUMBO_TAG_UNKNOWN || name), function node_qualified_tagname_is,
file parser.c, line 603.
make: *** [check] Abort trap: 6

With the fix:

[==========] 593 tests from 9 test cases ran.
[  PASSED  ] 593 tests.

Ruby suite, test/html5/*_test.rb: 2839 runs, 10321 assertions, 0 failures, 0 errors, 24 skips.

One thing this PR does not fix

The same option leaks memory on a different input. With stop_on_first_error = true and a parse
that exits mid-tag, the tokenizer's in-flight tag state leaks its attribute vector
(start_new_tag → gumbo_vector_init in vector.c:28 → gumbo_alloc in util.c:25). It
reproduces on <td><template></te and needs no fragment context. That is a separate cleanup path
and I have left it out to keep this change small. Happy to do it here or in a follow-up,
whichever you prefer.

The two early exits that are reachable from Ruby do not leak. Measured with ASan and LSan at
Nokogiri's defaults, across four fragment contexts: <div>×5000 returns
GUMBO_STATUS_TREE_TOO_DEEP and 5,000 attributes returns GUMBO_STATUS_TOO_MANY_ATTRIBUTES,
both with no leak.

Provenance

Found by coverage-guided fuzzing of gumbo_parse_with_options (libFuzzer, ASan and UBSan,
1,991,470 executions), which reported no other crash. 160 artifacts reduced to this one signature,
and 0 of the 160 reproduce with stop_on_first_error = false.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[stevecheckoway]

Thanks! This LGTM. I reproduced the crash without the patch fixes it.

I looked at the failing tests, I believe they're due to libiconv's server's instability. I imagine rebasing this PR on top of main would make those go green.

If you want to fix the memory leak too, that'd be wonderful.

[jeremy]

I rebased this branch on main. It now includes #3671, and all 167 checks are green.

I also looked at the memory leak you mentioned. My first analysis of it was wrong, and I want to correct the record before anyone acts on it.

What I measured

I built gumbo-parser/src standalone with -fsanitize=address and assertions enabled, and parsed <td><template></te as a document with stop_on_first_error = true. On aarch64-linux, LSan reports:

Direct leak of 8 byte(s) in 1 object(s):
  gumbo_alloc util.c:25  <- gumbo_vector_init vector.c:28  <- start_new_tag tokenizer.c:701
  <- handle_tag_open_state tokenizer.c:1106  <- gumbo_lex tokenizer.c:3446
  <- gumbo_parse_with_options parser.c

A parse with stop_on_first_error = false leaks nothing.

The leak is not in the tokenizer

The call chain points at the tokenizer, so I first assumed that the in-flight tag state leaks its attribute vector. That is incorrect. I instrumented gumbo_tokenizer_state_destroy. At teardown the tag state is already clean:

[DESTROY] attrs.data=(nil) len=0 name=(nil)

_attributes.data is NULL there. I wrote the obvious one-line fix anyway - free _tag_state._attributes in gumbo_tokenizer_state_destroy when _attributes.data is not NULL, which is what abandon_current_tag does - and I confirmed it does not fix the leak. The guard never fires. An unguarded free is worse: after a normal parse gumbo_string_buffer_destroy leaves _buffer.data dangling, so freeing without the guard causes a double free.

Where the leak actually is

The leaked vector belongs to a start tag. Its attributes were transferred to an element node, and the ownership assert in the tree construction loop passes. That node was then orphaned when stop_on_first_error ended the loop in the middle of tree construction:

  } while (
    (token.type != GUMBO_TOKEN_EOF || state->_reprocess_current_token)
    && !(options->stop_on_first_error && parser._output->document_error)
  );

gumbo_destroy_output frees the document tree. An element that is still on _open_elements, but that is not yet reachable from the document root, is never freed. This is the condition the comment above it already describes:

The parser is pretty fragile. Breaking out of the parsing loop in the middle of the parse can leave the document in an inconsistent state.

The fix, and why I am not pushing it

The tree depth limit a few lines above shows the safe pattern. It does not break out of the loop. It sets the token to EOF and lets finish_parsing unwind:

      if (unlikely(state->_open_elements.length > max_tree_depth)) {
        parser._output->status = GUMBO_STATUS_TREE_TOO_DEEP;
        token.type = GUMBO_TOKEN_EOF;
      }

If stop_on_first_error did the same, the existing cleanup would run and would free the orphaned elements. That is the correct fix, and it is small.

But it changes what stop_on_first_error means. Today it stops the parse immediately. With the EOF injection it finishes the tree and then stops. That is a behaviour change in a public C API option, so it is your call, not mine. It also touches the same fragile path as the crash fix in this PR.

I am happy to write that follow-up PR if you want those semantics. Tell me which you prefer and I will do it. If you would rather keep the abrupt stop, then the leak needs a different fix, and I do not have one I trust.

Scope

This is a C API path only. Nokogiri never sets stop_on_first_error, and the option has no Ruby keyword, so no Ruby code reaches the leak. Eight bytes per aborted parse, on an option nokogiri does not use.

[flavorjones]

Thanks for the fix, I'll review.