Skip to content

libxmlparser: fix unsafe XMLNode array handling - #29

Merged
jens-maus merged 1 commit into
mainfrom
pr/fix-libxmlparser-xmlnode-handling
Sep 8, 2026
Merged

libxmlparser: fix unsafe XMLNode array handling#29
jens-maus merged 1 commit into
mainfrom
pr/fix-libxmlparser-xmlnode-handling

Conversation

@jens-maus

@jens-maus jens-maus commented Sep 8, 2026

Copy link
Copy Markdown
Member

XMLNode has non-trivial copy and destruction semantics because it maintains a reference-counted XMLNodeData pointer. Moving its instances with realloc() or memmove() bypasses those semantics and is undefined behavior.

Allocate child arrays with new[] and relocate active nodes through their copy assignment operator. Track the allocated capacities of all internal arrays so shrinking them in exactMemory() remains compatible with later add operations. This also fixes a pre-existing heap overflow when a parsed node was modified afterwards.

Finally, check seek, size, allocation and fread results in parseFile() instead of parsing an uninitialized or partially filled buffer.

Summary by CodeRabbit

  • Bug Fixes

    • Improved XML parsing reliability during memory allocation and cleanup.
    • Added validation for file reading operations, with clearer errors when files are missing, unreadable, or cannot be loaded into memory.
    • Improved handling of large or complex XML documents to reduce allocation-related failures.
  • Maintenance

    • Updated the XML parser to version 1.14.

XMLNode has non-trivial copy and destruction semantics because it maintains a reference-counted XMLNodeData pointer. Moving its instances with realloc() or memmove() bypasses those semantics and is undefined behavior.

Allocate child arrays with new[] and relocate active nodes through their copy assignment operator. Track the allocated capacities of all internal arrays so shrinking them in exactMemory() remains compatible with later add operations. This also fixes a pre-existing heap overflow when a parsed node was modified afterwards.

Finally, check seek, size, allocation and fread results in parseFile() instead of parsing an uninitialized or partially filled buffer.
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 95afe38a-51bb-465b-851b-9f644b5cc6f3

📥 Commits

Reviewing files that changed from the base of the PR and between 1b58142 and 4d030be.

📒 Files selected for processing (2)
  • src/libxmlparser/xmlParser.cpp
  • src/libxmlparser/xmlParser.h

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The XML parser now tracks array capacities, resizes child arrays with new[] and delete[], compacts memory through shared helpers, validates file reads, and updates child destruction handling.

Changes

XML parser memory and file handling

Layer / File(s) Summary
Capacity-managed node storage
src/libxmlparser/xmlParser.h, src/libxmlparser/xmlParser.cpp
XMLNodeData stores capacities for each array. Allocation growth uses capacity-aware logic. Child arrays use resizeChildren.
Memory compaction and cleanup
src/libxmlparser/xmlParser.cpp
exactMemory uses shrinkMemory. Child destruction shifts wrapper pointers and deletes child arrays with delete[].
File read error handling
src/libxmlparser/xmlParser.cpp
parseFile validates file positioning, allocation, and read results, then reports parser errors on failure.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 4d030

This change makes XML node storage and cleanup array-compatible, preserves node lifetime semantics during relocation, and returns parser errors for file-read failures. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing unsafe XMLNode array handling, including the memory-management changes and heap-overflow fix.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr/fix-libxmlparser-xmlnode-handling

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jens-maus
jens-maus merged commit 7b30a69 into main Sep 8, 2026
5 checks passed
@jens-maus
jens-maus deleted the pr/fix-libxmlparser-xmlnode-handling branch September 8, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant