XMLPrinter::Write() error and possible overflow.

[willywa]

A very recent download (this week) has

void XMLPrinter::Write( const char* data, size_t size )
{
    if ( _fp ) {
        fwrite ( data , sizeof(char), size, _fp);
    }
    else {
        char* p = _buffer.PushArr( static_cast<int>(size) ) - 1;   // back up over the null terminator.
        memcpy( p, data, size );
        p[size] = 0;
    }
}

On many platforms, int is 32 bit but size_t is 64 bit. That static_cast(size) could result in an integer much smaller than the original size, meaning the later memcpy() has a good chance of trashing memory.

It may be that the tinyxml2 code would never call Write() with a size that large, but since Write is protected (not private), some class derived from XMLPrinter could do so.

It is not clear to me what the static_cast accomplishes, since PushArr expects a size_t argument.

[leethomason]

Could you put up a PR with a fix?

[willywa]

Could you put up a PR with a fix?

I'm not currently doing C++ development.

[LuridHound]

At the moment of commiting - https://github.com/leethomason/tinyxml2/commit/90e69c95ef73e969f37e914cca7e82a3544da4d4 - warning was valid.

Due to the another interface - T* PushArr( int count ) - https://github.com/leethomason/tinyxml2/blob/90e69c95ef73e969f37e914cca7e82a3544da4d4/tinyxml2.h#L219C5-L219C28

But later interface changed to - T* PushArr( size_t count ) - https://github.com/leethomason/tinyxml2/commit/eb3ab0df5d184b84eb283c0806f6abee71e3409b

Taking all this into account - I see no reason to keep this cast, because it is no longer needed.

[LuridHound]

@leethomason , created PR to address this issue - https://github.com/leethomason/tinyxml2/pull/1040

[LuridHound]

@willywa , commit that fixes it - already in master - https://github.com/leethomason/tinyxml2/commit/c4e29afeaf94033b637fb6b2392e03d788fe1439