Skip to content

PushText: reject a null text pointer instead of dereferencing (fixes #1031) - #1094

Open
AetherAI3 wants to merge 1 commit into
leethomason:masterfrom
AetherAI3:fix/1031-pushtext-null-guard
Open

AetherAI3 wants to merge 1 commit into
leethomason:masterfrom
AetherAI3:fix/1031-pushtext-null-guard

Conversation

@AetherAI3

Copy link
Copy Markdown

Fixes #1031.

Small one. XMLPrinter::PushText(const char* text, bool cdata) (tinyxml2.cpp:2868) passes text straight through to Write( text ) (which is Write(text, strlen(text)) per tinyxml2.h:2342) and to PrintString( text, true ) (which dereferences p = text at tinyxml2.cpp:2681). Neither guards against a null text; a caller passing nullptr (fuzz-shaped or otherwise malformed input) segfaults inside PrintString.

The reporter's ASan run in #1031 shows the OOB read; a minimal repro under clang + -fsanitize=address,undefined reproduces cleanly against master 8224e427:

XMLPrinter p;
p.OpenElement("root");
p.PushText((const char*)nullptr, false);
p.CloseElement();

Baseline (RED):

tinyxml2.cpp:2681:17: runtime error: load of null pointer of type 'const char'
AddressSanitizer: SEGV on unknown address 0x000000000000
#0 tinyxml2::XMLPrinter::PrintString(char const*, bool) tinyxml2.cpp:2681:17
#1 tinyxml2::XMLPrinter::PushText(char const*, bool) tinyxml2.cpp:2879:9
#2 main repro.cpp:9:7

Post-fix (GREEN): same repro runs cleanly, PushText becomes a no-op on nullptr, <root/> is emitted correctly.

Fix

Reject a null text early — TIXMLASSERT fires in debug builds (matches the project's precondition style at PushAttribute:2773) and an explicit return protects release builds:

void XMLPrinter::PushText( const char* text, bool cdata )
{
    TIXMLASSERT( text );
    if ( text == 0 ) {
        // Reject a null text pointer in release builds too:
        // Write() and PrintString() below would strlen(text) and deref.
        return;
    }
    _textDepth = _depth-1;
    ...
}

Verification

  • Baseline crashes as shown above.
  • Post-fix: same repro exits cleanly, output is <root/> as expected.
  • Smoke regression under ASan: multi-element document with PushText("hello world") and CDATA text round-trips correctly to the expected 102-byte serialization.

Diff

+6/-0 in one file, one function.

Base

Applied on top of master 8224e427b655b83dae5e2298f1e6919523a78737.

Scope note

PushComment, PushDeclaration, PushUnknown, and the two-arg PushAttribute(name, value) have the same missing-guard shape (they all forward to Write(const char*) which calls strlen). Those aren't touched here to keep the diff scoped to the reported bug, but they're worth a follow-up — happy to send a wider patch if that fits maintainer taste.

Credit

Reported by @wangziqi520 in #1031 with an AddressSanitizer trace. The report pins the exact function and root cause; the fix is the minimal guard that resolves it.

Note on AI assistance

I used an AI assistant to help trace the crash and draft this write-up. The code change and every claim above I verified myself against the built binary.

…eethomason#1031)

XMLPrinter::PushText(text, cdata) forwarded text straight to Write(text)
(which is Write(text, strlen(text))) and PrintString(text, true), neither of
which guarded a null pointer. A caller passing nullptr segfaulted in
PrintString when it dereferenced p = text.

Reject a null text pointer early -- TIXMLASSERT matches the precondition
style used at PushAttribute:2773, and the explicit return protects
release builds where assertions are stripped.

Verified under clang + -fsanitize=address,undefined against master
8224e42: without the fix, the reporter's PoC hits
'tinyxml2.cpp:2681:17: runtime error: load of null pointer of type
''const char''' and 'AddressSanitizer: SEGV on unknown address
0x000000000000' with a stack of PrintString <- PushText <- main.
With the fix the same PoC returns cleanly and the printer emits
'<root/>' as expected. Multi-element regression (PushText + CDATA)
round-trips correctly.

Fixes: leethomason#1031
Reported-by: wangziqi520 (github.com/wangziqi520)
Signed-off-by: Brandon Barrante <aetherai@aethersystems.net>
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.

Possible Bug: Global Buffer Overflow in tinyxml2::XMLPrinter::PushText(char const*, bool)

1 participant