Skip to content

Fix memory leaks in XML parser #8929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tautschnig wants to merge 1 commit into
base: develop
Choose a base branch
from
+12 −3
Open

Fix memory leaks in XML parser #8929

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/xmllang/parser.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ int yyxmlerror(xml_parsert &xml_parser, void *scanner, const std::string &error)
// unreachable code
#pragma warning(disable:4702)
#endif

// Bison-generated yydestruct only handles symbols with %destructor;
// suppress the warning about unhandled enum values in that switch.
#if defined(__GNUC__) || defined(__clang__)
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #pragma GCC diagnostic ignored "-Wswitch-enum" is applied for the remainder of the generated parser translation unit. To avoid masking unrelated switch-enum warnings in this file, consider using #pragma GCC diagnostic push before the ignore and a matching #pragma GCC diagnostic pop in the epilogue (third %% section) to scope the suppression.

Suggested change
#if defined(__GNUC__) || defined(__clang__)
#if defined(__GNUC__) || defined(__clang__)
#pragma GCC diagnostic push

Copilot uses AI. Check for mistakes.
#pragma GCC diagnostic ignored "-Wswitch-enum"
#endif
%}

%parse-param {xml_parsert &xml_parser}
Expand All @@ -33,10 +39,13 @@ int yyxmlerror(xml_parsert &xml_parser, void *scanner, const std::string &error)
%union {char *s;}

%token STARTXMLDECL
%token VERSION STARTPI ENDPI EQ SLASH CLOSE END
%token <s> ENCODING NAME VALUE DATA COMMENT START
%token VERSION ENDPI EQ SLASH CLOSE END
%token <s> ENCODING NAME VALUE DATA COMMENT START STARTPI
%type <s> name_opt

// Memory management: ensure allocated string tokens are freed during error recovery
%destructor { free($$); } ENCODING NAME VALUE DATA COMMENT START STARTPI name_opt
Comment on lines +46 to +47
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes error-recovery behavior by adding %destructor cleanup for string tokens. There are regression tests for --xml-interface (e.g., regression/cbmc/xml-interface1), but they only cover well-formed XML; consider adding a regression with malformed XML (ideally involving a PI like <?x without closing ?>) to exercise the error-recovery path and ensure cleanup doesn’t crash (e.g., due to double-free).

Suggested change
// Memory management: ensure allocated string tokens are freed during error recovery
%destructor { free($$); } ENCODING NAME VALUE DATA COMMENT START STARTPI name_opt
// Memory management is handled explicitly in semantic actions via free(...)
// No %destructor is used for string-valued symbols to avoid double-free.

Copilot uses AI. Check for mistakes.

Comment on lines +46 to +48
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding %destructor free($$) for string-valued symbols means any semantic value freed manually while the symbol is still on the parse stack (e.g., the mid-rule action in element : START { ... free($1); } ...) risks double-free during error recovery. To avoid this, only free these values once the symbol is no longer on the stack (end-of-rule action), or set the semantic value to nullptr after freeing.

Copilot uses AI. Check for mistakes.
%%

document
Expand Down Expand Up @@ -68,7 +77,7 @@ misc

PI
: STARTPI NAME
{ free($2); xml_parser.stack.push_back(&xml_parser.parse_tree.xml); }
{ free($1); free($2); xml_parser.stack.push_back(&xml_parser.parse_tree.xml); }
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new %destructor that frees STARTPI/NAME on error recovery, freeing $1/$2 in this mid-rule action can cause a double-free if a parse error happens later in this PI (the tokens are still on the parse stack and will be destroyed during error recovery). Move these frees to an end-of-rule action (after ENDPI) or null out the semantic values after freeing so the destructor becomes a no-op.

Suggested change
{ free($1); free($2); xml_parser.stack.push_back(&xml_parser.parse_tree.xml); }
{ xml_parser.stack.push_back(&xml_parser.parse_tree.xml); }

Copilot uses AI. Check for mistakes.
attribute_seq_opt
{ xml_parser.stack.pop_back(); }
ENDPI
Expand Down
Loading